TheOdinProject / curriculum

The open curriculum for learning web development
https://www.theodinproject.com/
Other
9.67k stars 13.11k forks source link

Passing Data Between Components: Inconsistency in the code example #28736

Open Lanut1 opened 1 week ago

Lanut1 commented 1 week ago

Checks

Describe your suggestion

In the section "Functions as props" there probably is an inconsistency in the second code example. The Button component hardcodes a URL, while the App component is made to accept different URLs. If I am not mistaken, the main goal is to make Button more reusable with different styles and links. To improve component reusability, consider updating the example to pass the URL as a prop to the Button component.

Here's a suggested version:

function Button({ text = "Click Me!", color = "blue", fontSize = 12, handleClick, url = 'https://www.theodinproject.com' }) {
  const buttonStyle = {
    color: color,
    fontSize: fontSize + "px"
  };

  return (
    <button onClick={() => handleClick(url)} style={buttonStyle}>
      {text}
    </button>
  );
}

export default function App() {
  const handleButtonClick = (url) => {
    window.location.href = url;
  };

  return (
    <div>
      <Button handleClick={handleButtonClick} url = 'https://react.dev/learn/passing-props-to-a-component' />
    </div>
  );
}

Path

Node / JS

Lesson Url

https://www.theodinproject.com/lessons/node-path-react-new-passing-data-between-components

(Optional) Discord Name

No response

(Optional) Additional Comments

No response

wise-king-sullyman commented 1 week ago

I'm not entirely sure what the original intent of the second example was, @bscottnz @01zulfi looks like yall did the original work on it, can you shine any light here?

bscottnz commented 1 week ago

I think the intention was to show how you can pass references to functions as props and call them within the child. Lanut1 is right here in that it doesn't really make sense to hard code the URL in the button, but I don't think passing the URL separately makes much sense either when you would instead just do handleClick={() => handleButtonClick(URL)}.

I could edit it to show both examples, one like the one I just mentioned and another which passes just the reference in a way that makes more sense?

Lanut1 commented 1 week ago

@bscottnz @wise-king-sullyman Thank you for the answers! For me, it would be really helpful to see how to correctly pass different URLs, as I was lost about this.

wise-king-sullyman commented 1 week ago

@bscottnz thanks! I agree that passing the URL as a separate prop feels odd here.

@Lanut1 like @bscottnz said a common approach to this sort of thing could be something like having the second examples handleButtonClick and passing it like:

<Button handleClick={() => handleButtonClick('https://www.theodinproject.com') />

Since you've both volunteered to work on this it might be a good idea to collaborate if you'd both be open to that, but if either of you would rather I just pick who puts the PR up I can do that too 🙂


This is really tangential but if you're curious, probably the most realistic but not relevant to what's trying to be demonstrated here approach:

You could have just a url or href or link prop on your Button, which when something is passed to it an anchor element (which just looks like a button) is actually rendered instead of a button. That's kind of what we do for this sort of application in the component library I maintain for work for example.

Lanut1 commented 1 week ago

@wise-king-sullyman thank you! This topic is more clear for me now! I'm very interested in learning how to create a PR, as I've never done one before.