I finished the reactjs tutorial and tried to make my first page.
I made a button to spawn boxes, and in each box buttons to move or remove them.
I made a version where the moving part did not work, and I still don’t understand why. I then asked bard for a more simple version and then extended it to fit my requirement. Can you help me understand why the first version is not working. Basically, the list of chart (charts), is never changed. The index of the chart is always -1, but my types look good to me…
Here is the not working code :
import { useState } from 'react';
import './App.css';
function TopSelect({ gpios, addChart }){
const options = []
function handleSubmit(e){
e.preventDefault();
const data = new FormData(e.target);
const formJson = Object.fromEntries(data.entries());
addChart(formJson.gpios);
}
gpios.forEach(e => {
options.push(<option key={e} value={e}>GPIO{e}</option>);
})
return <form onSubmit={handleSubmit}>
<label>
GPIO
</label>
<select name="gpios">
{options}
</select>
<button type="submit">Add chart</button>
</form>;
}
function Chart({ gpio, pos, removeChart, moveChart}){
return <li key={pos}>
<h3>GPIO{gpio}</h3>
<button onClick={()=>moveChart(pos, false)}>Up</button>
<button onClick={()=>moveChart(pos, true)}>Down</button>
<button onClick={()=>removeChart(pos)}>Remove</button>
</li>;
}
function App() {
const [ gpios, setGpios ] = useState([]);
const [ charts, setCharts ] = useState([]);
const [ gpioInitialized, setGpioInitialized ] = useState(false);
function initGpio(){
let newGpio = [];
[1, 4, 7, 12].forEach(x => (
newGpio.push(x)
));
setGpios(newGpio);
}
function addChart(gpio){
let pos = 0;
if(charts.length > 0) pos = Math.max(...charts.map(c => c.id)) + 1;
const newChart = {id: pos ,
elem : <Chart gpio={gpio} pos={pos} removeChart={removeChart} moveChart={moveChart}/>};
setCharts([...charts, newChart ]);
}
function removeChart(pos){
setCharts((current) => current.filter((chart) => chart.id !== pos));
}
function moveChart(pos, down){
const index = charts.findIndex((chart) => chart.id === pos);
console.log(index);
if(!down){
if (index <= 0) return;
setCharts((charts) => {
const chartsCopy = [...charts];
const [chart] = chartsCopy.splice(index, 1);
chartsCopy.splice(index - 1, 0, chart);
});
}else{
if (index >= charts.length - 1) return;
setCharts((charts) => {
const chartsCopy = charts.slice();
const [chart] = chartsCopy.splice(index + 1, 1);
chartsCopy.splice(index, 0, chart);
});
}
}
if(!gpioInitialized){
initGpio();
setGpioInitialized(true);
}
return (
<div className="App">
<TopSelect gpios={gpios} addChart={addChart} />
<ul>
{
charts.map((chart) =>
chart.elem
)
}
</ul>
</div>
);
}
export default App;
And here is the working version :
import React, { useState } from "react";
function Menu({ gpios, addBox }){
const options = []
function handleSubmit(e){
e.preventDefault();
const data = new FormData(e.target);
const formJson = Object.fromEntries(data.entries());
addBox(formJson.gpios);
}
gpios.forEach(e => {
options.push(<option key={e} value={e}>GPIO{e}</option>);
})
return <form onSubmit={handleSubmit}>
<label>
GPIO
</label>
<select name="gpios">
{options}
</select>
<button type="submit">Add chart</button>
</form>;
}
function Box({ id, gpio, removeBox, moveBoxUp, moveBoxDown}){
const boxStyle = {
width: "100%",
height: "100px",
margin: "10px 0",
padding: "10px",
border: "1px solid black",
backgroundColor: "white",
};
let buttonUp = "";
let buttonDown = "";
if(moveBoxDown){
buttonDown = <button
onClick={() => moveBoxDown(id)}
>
Move Down
</button>;
}
if(moveBoxUp){
buttonUp = <button
onClick={() => moveBoxUp(id)}
>
Move Up
</button>;
}
return <li key={id}>
<div style={boxStyle}>
<h1>GPIO {gpio}</h1>
<button
onClick={() => removeBox(id)}
>
Remove
</button>
{buttonUp}
{buttonDown}
</div>
</li>;
}
function App(){
const [gpios, setGpios] = useState([]);
const [gpioInitialized, setGpioInitialized] = useState(false);
const [boxes, setBoxes] = useState([]);
function initGpios(){
if(gpioInitialized) return;
const available = [1,4,7,13];
setGpios(available);
setGpioInitialized(true);
}
function addBox(gpio){
const newBox = {
id: Date.now(),
gpio: gpio,
};
setBoxes([...boxes, newBox]);
};
function removeBox(id){
setBoxes(boxes.filter((box) => box.id !== id));
}
function moveBoxUp(id){
const index = boxes.findIndex((box) => box.id === id);
if (index > 0) {
setBoxes((boxes) => {
const boxesCopy = boxes.slice();
const [box] = boxesCopy.splice(index, 1);
boxesCopy.splice(index - 1, 0, box);
return boxesCopy;
});
}
}
function moveBoxDown(id){
const index = boxes.findIndex((box) => box.id === id);
if (index < boxes.length - 1) {
setBoxes((boxes) => {
const boxesCopy = boxes.slice();
const [box] = boxesCopy.splice(index + 1, 1);
boxesCopy.splice(index, 0, box);
return boxesCopy;
});
}
};
initGpios();
return (
<div>
<Menu gpios={gpios} addBox={addBox} />
<ul>
{boxes.map((box, index) => (
<Box id={box.id} gpio={box.gpio} removeBox={removeBox} moveBoxDown={(index===boxes.length-1)?null:moveBoxDown} moveBoxUp={(index===0)?null:moveBoxUp}/>
))}
</ul>
</div>
);
}
export default App;
With the new version, boxes move perfectly..
Thanks !
PS: If you have any tip or if some lines are burning your eyes, please tell me !
2
Answers
I’ve gone through your code and it looks well structured, nice work for a beginner, I must say.
I have a few points that you can consider to further improve your code:
Start using
prop-types
(https://www.freecodecamp.org/news/how-to-use-proptypes-in-react/) or typescript (this might take a lot of time to learn), but I think for now you can go ahead withprop-types
please. This helps to catch errors very easily.I can see that you’ve used
index
askey
, try using a unique identifier for exampleid
.I think inline-styles should be avoided, that way your code is going to be much cleaner.
I believe that your code can be further broken into multiple new components.
Try having consistent variable names please. I can see you have
boxes
andbox
both.Good start as learner though! I encourage you to learn and grow more.
<button onClick={()=>moveChart(pos, false)}>Up</button>
, this is not working as expected. What is happening is amoveChart
function is somehow getting registered (while declaring this onclick) and all the values that this function uses – includingcharts
, are getting registered with it too.So after adding 5 charts, you are moving one of the chart up, the
moveChart
function will have an arraycharts
of length 1 instead of the expected 5 and hence things are not working as expected. Why this happens is something new to me too, let me know if you find it.You can test this by declaring a new state. Add all your charts once and then try moving them. Ideally, you should be getting the same output for
val
, but the value that you are getting is the one at the time of declaring thechart
and theonClick
itself.const [val, setVal] = useState(-5);
Incrementing the state in the
addChart
function,And printing it inside the
moveChart
function –onClick
formove
is already defined when declaring achart
, so even if the position changes, the function calls formove
have the oldpos
as their parameter.id
which you used to determine their location doesn’t change. Thus they are now mapped to the wrong locations.chart
is not used anywhere I guess,const [chart] = chartsCopy.splice(index + 1, 1);
These are some of the things that I could find, there maybe some more.