Conversation
dannebrob
left a comment
dannebrob
left a comment
There was a problem hiding this comment.
Nice work Vio, be proud! You finished the black-lvl, great work!
- As we discussed over the week, comments can be hard to get right. So I left some feedback on some that catched my eye.
- I'm missing some meta data in the index.html (in public folder), its always nice.
- There are some responsive issues on chrome, not sure if you gotten the width px from the UX designer, mostly close to the break-points, with buttons and carusell. Images and descriptions below.
Missing some spacing on the smallest screens of the buttons
Overlapping divs and also some overflow?
Other than that I have nothing to add, except to repeat my self (This is not DRY) great work on the project.
| const StyledAnnualBox = styled.div` | ||
| background-color: ${props => props.selected ? "#F4E4D7" : "#D0C4B8"}; | ||
| box-shadow: 0px 4px 16px rgba(0, 0, 0, 0.25); | ||
| border-radius: 12px; | ||
| width: ${props => props.selected ? "342px" : "304px"}; | ||
| margin-top: 35px; | ||
| }; |
| I wish I had more time to implement more funtions. | ||
| The onclick from different components, for example! */ |
There was a problem hiding this comment.
This is a comment that really tells a story; maybe you could think who are you writing a comment to Is it for yourself or someone to continue on your code? My personal preference is to keep it short and informative.
There was a problem hiding this comment.
Thanks! This is a comment to myself mostly, for when I come back to implement some of the things I set up for but didn't have time to finish during the week. I know I will have forgotten a lot so the comments are long to help me remember 🙂
| ` | ||
|
|
||
| const Nutrition = () => { | ||
| const isMobile = useMediaQuery('(max-width: 767px)'); // Hook to check screen size. |
There was a problem hiding this comment.
Nice use of useMediaQuery, I did not know about it before!
| /* Rendering the different titles depending on screen size. I think this could be done with | ||
| props and a title component, but I've not gotten it to work on resize of screen, | ||
| only on first render of site. | ||
| Instead I have installed an npm package to solve this. Might not be the best solution. */ |
There was a problem hiding this comment.
NPM packages are there to help right? I would say that this is a great solution. But if you want you could do some css magic with ::before and media queries in css (use the content property to specify the content to insert depending on screen size), but I think this is a cleaner way that you have done it.
|
|
||
| const vidRefs = useRef([]); | ||
|
|
||
| const handleToggleVideo = (index) => { |
There was a problem hiding this comment.
I'm not 100% sure how this vidRef is working, this might be a good place to insert a comment.
|
Thank you for taking the time to go through the code! 🌟 |
2 participants
https://spectacular-pudding-f8c7fa.netlify.app/