so cool!
Design Comparison
SolutionDesign
Report
1Accessibility
issues1HTML
issues
Aaron Stroud's questions for the community
π I will be updating this app (refactoring state using Redux, writing more tests), so I am interested in any and all feedback!
- The timer mode is controlled by a form containing three radio inputs. There's no submit button because the form is never meant to be submitted. What's the best approach to this accessibility issue?
- Why is Firefox rendering my colors differently than Chrome/Safari?
- Is there too much transition bling on the buttons?
Community Feedback
- 1
- 0
- 3
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 . 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 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 is much different than a heading so doesn't match the name of the component at all. And for your mute toggle button, ideally you should be able to use a 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!
- 0
- 1
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
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
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: cursorto the clickable elements of the site. πOf course, keep coding (and happy coding, too)! π
- 0
- 1
- 1
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
buttonelements 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! π
- 0
- 1
@astroud no worries! It was well deserved π
P
P
P
PGive some feedback to astroud about their solution
Join our Slack community
Join over 45,000 people taking the challenges, talking about their code, helping each other, and chatting about all things front-end!