skip to Main Content

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


  1. 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:

    1. 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 with prop-types please. This helps to catch errors very easily.

    2. I can see that you’ve used index as key, try using a unique identifier for example id.

    3. I think inline-styles should be avoided, that way your code is going to be much cleaner.

    4. I believe that your code can be further broken into multiple new components.

    5. Try having consistent variable names please. I can see you have boxes and box both.

    Good start as learner though! I encourage you to learn and grow more.

    Login or Signup to reply.
    • <button onClick={()=>moveChart(pos, false)}>Up</button>, this is not working as expected. What is happening is a moveChart function is somehow getting registered (while declaring this onclick) and all the values that this function uses – including charts, 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 array charts 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 the chart and the onClick itself.

    const [val, setVal] = useState(-5);

    Incrementing the state in the addChart function,

    setVal((val) => val + 5);
    setCharts([...charts, newChart]);
    

    And printing it inside the moveChart function –

    function moveChart(pos, down) {
        console.log(pos, charts, val);
    

    • What happens on onClick for move is already defined when declaring a chart, so even if the position changes, the function calls for move have the old pos as their parameter.
    • When moving boxes, their id which you used to determine their location doesn’t change. Thus they are now mapped to the wrong locations.
    • Also, try adding comments to all your code.
    • 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.

    Login or Signup to reply.
Please signup or login to give your own answer.
Back To Top
Search