Todo app | React | styled-components | Drag and Drop

Community feedback

• 

  

  ApplePieGiraffe•30,525

  @ApplePieGiraffe

  Posted over 4 years ago

  Hey there, Jen! 👋

  Nice to see you complete another challenge! 😀 Great job on this one! 👏 Your solution looks good and the to-do list functions well! 👍

  I'm afraid I can't offer much advice about React (since I'm rather new to it myself), so I just suggest adding some labels to the interactive elements of the page to get rid of a few errors on your solution report and make your solution more accessible. Adding some outlines to things for when items are focused, too, would be a nice addition for keyboard users! 😉

  Also, on my screen, there's a horizontal scroll bar that appears along the bottom of the page when the height of the to-do list becomes higher than what can fit in my viewport. IDK why that is but adding something like overflow-x: hidden to the body should get rid of it! 🙂

  Keep coding (and happy coding, too)! 😁

  21
• 

  

  Matt Studdert•13,611

  @mattstuddert

  Posted over 4 years ago

  Great to see you post another project, Jen! How have you been getting on with React so far? It definitely takes on a new level of complexity as you build larger projects!

  You've done an excellent job on this challenge. Your app looks great and functions as expected based on the project brief. Here are some thoughts after taking a look at your code:

  • I'm not a fan of the useViewport() hook. Having a resize listener constantly running on the window isn't something you want to do if it can be avoided. Is there a reason you opted for this approach as opposed to using a media query?
  • In the App.js file, you're defining a saveLocalTodos function and then calling it immediately. As you're calling it straight away, you don't need the definition at all. So you can remove the function and just set the items in the useEffect hook without it being wrapped in anything.
  • Don't only show the delete todo button on hover. As it is, people navigating using a keyboard can't delete todos. I'd recommend trying to navigate and complete tasks using only your keyboard on any projects you build. This will give you a good insight into whether or not your project is navigable by keyboard users.
  • As Steven mentioned, avoid setting height and width properties, except for particular circumstances where you have an obvious reason. Typically, you want the inner content of an element to dictate the dimensions of an element. For example, you've set a height on the todos. Try typing out a really long todo and see what happens. Ideally, you'd want the todo to grow with the content.
  • You've got Main.js and ItemBox.js in their own file, but they're not actually doing anything other than being exported. If you're creating a component, I would recommend trying to keep all concerns for that component locally in the file. I'd recommend reading this "styled-components happy path article by Josh Comeau". It will hopefully give you some more ideas on this. As with anything, keep practising, and you'll develop better habits as you become more experienced with the tool.
  • As Steven mentioned, the idea of React is to componentize code as much as possible to maximise reusability. An example of where you can break down a component further is the Todo.js. This could be broken up into smaller chunks, like the checkbox, text, and delete button.

  I hope this helps. Keep at it and let me know if you have any questions! 🙂

  12
• 

  Steven Toben•750

  @steventoben

  Posted over 4 years ago

  This honestly barely looks like a React app to me. The styled components is completely unnecessary and makes the code almost unreadable. Some of your components aren't even React components, it's literally CSS applied to an HTML element in a JS file. That's a massive anti-pattern. You want to separate every part of an app as much as possible. Your React components don't really make sense to me, the point of React is atomic design, allowing for reusable components. You could have made a button component that would be used for all other instances of a button you had throughout the code. You should separate components by domain, down to the most basic level and build up. Regarding styling, I already say this to every single person on here so I'll keep it short; don't set your font-size in the html tag unless you set it to 100%(which is the same as doing nothing). And then of course I have to ask, where did your numbers come from? You heights, widths (which usually shouldn't be set at all), padding, margin, etc. The numbers look completely random, CSS requires logic to properly layout a design and make it maintainable. If you change a couple numbers the entire design is ruined. Also a lot of those effect hooks look like they aren't really cleaning up properly. Last thing is don't use flexbox for everything. There's a lot of hacky html with incorrect semantics, as well as hacky css, I'd try to make sure everything is properly setup you're not relying on unstable solutions

  03