Todo List - Project Feedback Please!

Code Feedback
1

Hey guys,

This isn’t a FCC project but I would highly appreciate if you give me ANY feedback on this project.

https://put-em-down.netlify.com/

Here is a link to my code in case anyone wants to look at it. https://github.com/shimphillip/put-em-down

Thank you!

2

Complete and absolute noob here, so I can’t give constructive comment on technicalities and code (sorry), but I think it looks super nice!

3

What technologies did you use to build it.

4

@topcoder

"react": "^16.8.3",
"styled-components": "^4.1.3"

@shimphillip

const { state, toggleCompleted, removeTodo, handleVisibility, handlePriorityVisibility, addTodo } = this;
const props = { state, toggleCompleted, removeTodo, handleVisibility, handlePriorityVisibility, addTodo };    
<Dashboard {...props} />

You are spreading all properties into variables, then you make an object of them and call it props.

You could write <Dashboard {...this} /> and delete the two other lines.
This would save around 10 formatted lines & improve the readability “Oh, he passes all class properties to Dashboard” vs. “WTF is he doing?”.
No need for thinking about performance.

Looking good.

5

@miku86
Hey thanks! I knew about this and was debating.

Like you said, I could simply do {…this} and pass down props like that but I noticed that it also passes down other built-in methods of this besides the ones that I coded up.

So does performance barely get any hit if any even if I pass down props that child components won’t use?

6

@topcoder

:arrow_up: comment and for fading effects I used react-reveal library.

7

@pipestream Hey thanks for the compliment!

8

Don’t do this. You should pass only stuff you use not because of performance concerns (I doubt there are any), but because when you use it in combination with stuff like prop-types or TypeScript types/interfaces you’ll get nice warnings when you forget to pass a prop or pass a wrong one.

The way you did it was horrible. “WTF is he doing?” was my exact thoughts.

Btw, kudos for using hooks :wink: And I see you gave up on Transitions :unamused:

9

How would you solve it?

For using Typescript, I think in this rare case we can use an any,
instead of sacrificing readability by passing 6 or more props.

10

I would get rid of separate Dasboard component altogether (it’s just styling), put Header and Todos in App and pass relevant props directly by doing verbose, but more readable <Todos todos={todos} visible addTodo={} ... />.
And prettier will format all 6 (or whatever) props nicely.

For TS I would create an interface that describes passed props to each component. With relevant type. No any!!!

1 Like
11

That sounds like a good approach. What about a scenario where there are a lot more props to pass down like 10? and looks ridiculous. Would you still do what you suggested? Or is that a point where I start breaking it into smaller components? I think using Redux would not be an option since it’s just one or two components down.

12

Hard to tell. There is no “one-size-fits-all” solution. One approach is to keep as may as possible components in parent components.

Instead of:

App.js
<App>
  <Dashboard props1 props2 props3 props4/>
</App>

Dashboard.js
<div>
  <Header props1>
  <Tods props2 props3 />
  <Footer props4 />
</div>

you can do:

<App>
  <div>
    <Header props1>
    <Tods props2 props3 />
    <Footer props4 />
  </div>
</App>

You could check popular projects on github and see every possible approach.

1 Like
13

@jenovs

Thanks, it makes sense.

Probably not a good idea to treat styled-component style like a react component and move nested react components up one level will be better.