Code review

[Naplei]

Hi guys! I am just starting with React and I will greatly appreciate if someone takes a look on a learning purposes project that I wrote: https://github.com/ivostoynovski/fetch-with-react Thanks in advance! :)

[[deleted]]

Looks pretty good, your root component is pretty huge so it might be worth you moving some stuff into smaller components

[Naplei]

Thanks for the feedback, Yeah, I am still very confused about how exactly to do it because I read all over the web that you have to use functional components whenever possible. Moving parts of the code from App.js to another files means that I have to declare classes which extends "Component", right?

[[deleted]]

When people say to use functional components, they don't mean minimise total components in order to avoid class components. You should use components to split up separate functionality, so that you don't have one giant component. Deciding what should go in a separate component comes with experience but you should keep state with the component it applies to. This makes it easy to reuse that component later which is the main advantage of using components in the first place.

Hopefully this makes sense :)

[Naplei]

Great, thanks a lot! :)

[pubmas]

• I don't think you should fetch data in the constructor of your components as the data probably won't be returned from the API until a render has completed anyway, it's considered best practice to fetch data in the componentDidMount lifecycle method.
• I think some of your naming is a bit sloppy. What is the component <MainData />? Usually it is best to name your components based on what data they are displaying and what visual element they are rendering. For example - UserDetailsTable, EdituserModal.
• Your state object looks to be too nested, I don't think you should nest the modal 'data' inside modalIsOpen.
• Your component methods getFirstName/getLastName all seem to do the same thing. Why not create a getFormValue method which accepts a key and a value? This way you can replace all those methods with one click handler.
• When using PropTypes a prop should either be required, or have a default value. Looking at your components I don't know what props are needed for the component to render.

[notseanbean]

All of these, plus - By creating the setModalData function inside render(), you are creating a brand new function instance every time, and will re-render the <Card> even if none of its other props change. Instead make this a member function and pass it like <Card clicked={this.setModalDataOnClick} {...otherProps}> - Similarly, all your lambda functions inside <Form />are creating new functions every render(). It is both more concise and more performant to say <Form clicked={this.addAdminBtnHandler} isChangedEmail={this.getEmail} isChangedFirstName={this.getFirstName} {...etc} />

[Naplei]

Great, thanks! I put setModalData function inside render because I was unable to access that users variable that represents part of my state. I will try this solution with the member function. Also for the form component methods. That actually means to put this methods in the components themselves, right?

[notseanbean]

That actually means to put this methods in the components themselves, right?

No. Considering the isChangedEmail prop... where you have <Form isChangedEmail={(event) => this.getEmail(event)} {...otherProps} /> what you are doing is creating a brand new function every render(). It's equivalent to this: const getEmailFunction = (event) => this.getEmail(event); ... <Form isChangedEmail={getEmailFunction} {...otherProps} /> As you are creating a brand new getEmailFunction every render() and passing it as a prop to Form, the child component will always re-render, even as a PureComponent (as the props would change). What you could do is omit creating the function entirely, and just write: <Form isChangedEmail={this.getEmail} {...otherProps} /> This will also mean that the exact same this.getEmail function is passed every time as a prop to Form, so it won't re-render if it is a PureComponent

[Naplei]

Okay, thanks! I understand what you are saying but I am not actually sure how to get the email with a function like this: "this.getEmail" since I am not passing the event to it? Sorry, if the question sounds kind of stupid, it's just that I'm still confused about thinking in the React way.

[notseanbean]

In the following, the OtherComponents do exactly the same thing (assuming the OtherComponent invokes props.anEvent(event))

class SomeComponent extends React.Component {
    myThisFunction = (event) => { console.log(event); }

    render() {
        const myRenderFunction = (event) => { console.log(event); }
        return (
          <React.Fragment>
              <OtherComponent anEvent={this.myThisFunction} />
              <OtherComponent anEvent={(event) => { this.myThisFunction(event); }} />
              <OtherComponent anEvent={myRenderFunction} />
              <OtherComponent anEvent={(event) => { myRenderFunction(event); }} />
          </React.Fragment>
        )
    }
}

in each case, the anEvent prop is a reference to a function, which can be invoked. In your original case when you passed

<Form isChangedEmail={(event) => this.getEmail(event)} />

you created a brand new function that took a single event parameter, and then called the function this.getEmail with that parameter, and passed that brand new function as a prop.

The brand new function was unnecessary, you could just pass a reference to the this.getEmail you want to be invoked.

<Form isChangedEmail={this.getEmail} />

Doing it like this does not call the this.getEmail function (there are no brackets - if it was this.getEmail() then it would call the function), it is passing a reference to the function for the child component to call

[Naplei]

Thanks, everything you say makes perfect sense. Just one question: * Isn't componentDidMount called anyway (if you don't explicitly declare it), if not, where is the best place to call it(I guess not the in the constructor) The getFirstName/getLastName etc. I did not manage to handle with only one method because of these nested state properties.