Pomodoro React App

Community feedback

• 

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  Ok so in terms of React code quality, you should definitely work on code-splitting. In React a functional component really shouldn't be more than a few lines. So I'd recommend making more things into components, like you had a couple button html elements, those should be able to be a <Button/>. Speaking of that component, a Button component should return a button or anchor html element, on one of your cases your Button component returned a div with an input as a child, and your default return is null. I would at the very least just return a button instead of null, but even better you should use PropTypes to validate props and provide defaults. Another thing you could do to make your code cleaner is to make functions for each type of button in your Button.jsx, like a CloseButton, SettingsButton, etc. above your main Button function. Then you could check your prop and return <CloseButton/> or something like that all in one file. Another thing is making separate functions for each button, but before your export default Button line write Button.Close = CloseButton; Button.Settings = SettingButton; That would let you call a specific button from outside the file using dot notation, like you could call <Button.Settings {...props}/> in some other file to display that specific button. Last thing on buttons is you should style them with the line cursor: pointer; that way when you hover over them they appear much more clickable.

  Like I said before try to code-split, you could turn your radio inputs into their own Component. The Header component doesn't seem right to me, a <header> is much different than a heading so <h1> doesn't match the name of the component at all. And for your mute toggle button, ideally you should be able to use a <Button/> to render that one.

  For settings there's a lot going on here. It looks like you could make your own Text component, Input components, Label components, Form component. Pretty clever way of toggling the visibility of the settings modal, but it could be made even simpler by using a portal. The ReactDOM.createPortal() function lets you render whatever you put into the function, anywhere you want, so you can render things to document.body and then you'll have an element that is created as a sibling to your main VDOM tree.

  So another thing is when you use one of React's synthetic events, like onClick and set it to an external function like you did with onClick={handleClick}, you need to handle state more carefully. Creating an external function is completely ok but when inside the function you shouldn't write setIsActive(!isActive); instead you should write something like setIsActive((prevState)=>{return !prevState}); This seems odd but React's state updates are async so sometimes it takes a while to change the state and you could be working with the wrong state. So using the previous state is a nice workaround. Also you might want to think about using a ref for some of your Component variables. If you have a variable that you declare in a component that you want to persist through rerenders, and is really only used for storing some values, like colors, React's useRef hook lets you create instance variables which is helpful because on a rerender, React will recreate everything. Every function, variable, etc, is recreated unless persisted in a ref.

  So you mentioned you'll be updating the state with Redux, which is fine although I don't think it's really needed for this project. When you do implement redux, make sure not to do the same thing you did on this project. Do not make every single piece of state global. Most state in an app should be local, or hoisted up a layer, but most of the time global management for a state is unnecessary. I think you could use the Context hooks in React for any global state you do need, then send the state as far down the tree as possible. Ideally your state should be kept as close to the component as possible.

  As for CSS, I'd also look into code-splitting. That's a pretty massive file and it's a very common approach to create a style sheet for each component, as well a global one. I'll just comment on a couple things here, avoid magic numbers, numbers that look completely random and have no meaning except for the fact that they "just work". Try not to be so general with selectors, like don't style an h1 element unless it's for a reset, use class names. I highly recommend you avoid pixels at all costs, trade them for rem units, it will make your app much more accessible and responsive. At the very least use rem units when setting a font-size, ideally they should be used for dimensions, margins, padding, etc. And I think that the animation spin on the buttons is a bit too much, try to keep animations subtle and quick, ideally in the 100-400ms range, probably never past 900ms.

  Overall though the project looks nice, I would recommend taking these things into account for future projects though, maintainability could get out of hand pretty quick otherwise. Good job it looks pretty great overall!

  3

  ---

  

  Aaron Stroud•435

  @astroudPosted almost 4 years ago

  Wow @steventoben, thanks for this feedback gold!

  I'm going to break this down into a series of actionable tasks. I'm ready to get to work!

  0
• 

  T

  Matt Studdert•13,611

  @mattstuddertPosted almost 4 years ago

  Excellent work on this challenge, Aaron! You've done an outstanding job of reproducing the design, and I like the extras you've added in, like the sound.

  Using radio inputs to change the timer is fine. You can see that you can navigate them with the direction arrows. I'd add a focus state, as it's not obvious when it's focused when navigating using the keyboard. Another approach would be to have button elements that change the state and, therefore, the timer.

  Different browsers may render colours differently. There's nothing that can be done, so don't worry about it.

  Keep up the brilliant work! 👍

  1

  ---

  

  Aaron Stroud•435

  @astroudPosted almost 4 years ago

  Thanks for the suggestions @mattstuddert and for the highlight in the newsletter!

  0

  

  T

  Matt Studdert•13,611

  @mattstuddertPosted almost 4 years ago

  @astroud no worries! It was well deserved 🙌

  1
• 

  T

  Chamu•13,340

  @ChamuMutezvaPosted almost 4 years ago

  Good job indeed. Nice well written readme.md file, something that I need to improve on my side.

  1

  ---

  

  Aaron Stroud•435

  @astroudPosted almost 4 years ago

  Thanks @ChamuMutezva . One lesson that actually surprised me a bit was how much I dreaded working on the readme at the end. I wasn't expecting that. Next time I'm going to work on the readme in parallel with the code so it's not hanging over my head at the end.

  0
• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 4 years ago

  Hi, Aaron Stroud! 👋

  Great job on this challenge! 👏 Everything looks good and the timer works well! 👍 The transitions you added to the various interactive elements are also a nice touch! 😀

  I only suggest perhaps adding pointer: cursor to the clickable elements of the site. 😉

  Of course, keep coding (and happy coding, too)! 😁

  1

  ---

  

  Aaron Stroud•435

  @astroudPosted almost 4 years ago

  Thanks for the kind words and cursor tip @ApplePieGiraffe !

  0
• 

  P

  Emmilie Estabillo•5,540

  @emestabilloPosted almost 4 years ago

  Looks great Aaron! Bookmarking this, hope to also implement in the future 🙂 I’m viewing the project on a smaller screen laptop and I can’t scroll down to see the entire Settings pane. The same in ios, I can only see up to the top of the ‘Apply’ button. Otherwise, awesome 👏

  1

  ---

  

  Aaron Stroud•435

  @astroudPosted almost 4 years ago

  Thanks @emestabillo! This was a fun project. I knew there were some layout issues on small screens, but I really needed to get a good enough version released that I could improve upon it.

  I'm going to be refactor much (most?) of the project as @steventoben described. That'll give me a chance to make the layout fully responsive.

  1
• 

  Achraf EL KHANDOULI•65

  @AchrafLancePosted almost 4 years ago

  so cool!

  1

  ---

  

  Aaron Stroud•435

  @astroudPosted almost 4 years ago

  Thanks @AchrafLance . It was a great project—so many lessons learned. I'm looking forward to starting the next one (REST Countries API with color theme switcher).

  Are you working on one at the moment?

  0