Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReactJS Code style #127

Open
blatinier opened this issue May 9, 2020 · 0 comments
Open

ReactJS Code style #127

blatinier opened this issue May 9, 2020 · 0 comments

Comments

@blatinier
Copy link

Some comments on code style in React

Avoid some rendering with useCallback

This component will update the virtual DOM (and trigger a new render):

<ProposedLinks link={feed.link} links={feed.links}
    onChange={(e) => setState({ ...state, link: e.target.value})}
/>

Because each time react goes there it recreate the func which change id thus registering a change in the component.
useCallback allows you to avoid that by caching your function and recreating it only if its dependencies changes (a dependency list you should explicitly write).
Read more here: https://reactjs.org/docs/hooks-reference.html#usecallback

Do not generate key based on array index

Keys based on index are not stable identifiers for your components and can lead to fucked up bugs. Example you used:

            {links.map((proposedLink) => (
              <MenuItem key={"l" + links.indexOf(proposedLink)}
                value={proposedLink}>{proposedLink}</MenuItem>
            ))}

A change in the list (like replacing one element in the array) might not change any key and then not trigger any rendering (leading to a difference between the data in your state and the data rendered)
Using the value in the key is the way to go. (Note: generating an uuid on the fly is not better since it will change your keys even if nothing has changed. It's a common "workaround")
Read more here: https://reactjs.org/docs/lists-and-keys.html#keys

Boolean props

<StateTextInput required={true} />

Can be changed to

<StateTextInput required />

A rule often enforced in linters

Use template strings

key={"cat-" + cat.id}

Can be changed to

key={`cat-${cat.id}`}

This is better since you avoid the + and use a mechanism dedicated to string.
Usually enforced in linters.

One component per file

Yup all in the title :D

Component declaration

function ProposedLinks({ link, links, onChange }) {
  ...
  return (<Blablabla />);
}

Is usually declared like this:

const ProposedLinks = ({ link, links, onChange }) => {
  ...
  return (<Blablabla />);
}

Not a revolution, but just the way most ReactJS developers write func components

Escape quote and double quotes in JSX

<Alert severity="info">"{state["feed_type"]}" type doesn't ...</Alert>

Changed to

<Alert severity="info">&quote;{state["feed_type"]}&quote; type doesn&apos;t ...</Alert>

More hooks

You used some hooks but kept some HOC, try to make it uniform:
react-redux connectuseDispatch & useSelector
Read more: https://react-redux.js.org/next/api/hooks

Iterators

const commands = Object.keys(commandsDefs).map((key) => {
...
{commandsDefs[key].icon}

You should use Object.entries
Read more: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries

That's all for now. I didn't went throught all the code though. I hope you enjoyed the reading :)
All the best guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant