Coffeeroaster Multi Page | Next.js , Mobile first

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  Hi, Raymart! 👋

  Congratulations on finishing this challenge! Nice work on this challenge! 👏

  It looks good on both my mobile and desktop devices. Good job! 🙌

  Some feedback:

  • I notice that the About Us page doesn't contain any h1. I would recommend making the About Us as a h1.
  • Usually, the logo is wrapped with a link that will navigate the user to the home page. I would suggest doing that.
  • I try your website in prefers-reduced-motion and I still see the animation when I scroll and the transition on hover. I would expect that there will be no animation and transition.
  • I also try to see the site when the JavaScript is disabled and as a result, I only see content in footer and header. The text content inside the main element is gone. I would recommend having a noscript that will tell the user to enable JavaScript.

  I hope this helps! Keep up the good work! 👍

  Marked as helpful

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @vanzasetia Hi Vanza! Glad that you liked the site!

  For the feedbacks:

  • Glad that you noticed that one!! I mistakenly assume that I used the same component for each of the hero section of the page to hold the h1 text, but used a different one of the about us page. I was confused at this at first since the h1 text on the design is not same for each one so I had to use other stylings for the about page. Thanks for this!
  • For the logo part, I intended that one to not be on a link since the home link is already present on the navbar. Whenever I have a navlink that contains the homepage link, I don't wrap the logo in a link to avoid a duplicate aria-current="page" being announced 2 times when traversing links :>>
  • Apparently, framer-motion, the animation library that I used doesn't really removed all animations even if the user toggles the system's animation, the libray will still add the opacity transition for each one of the component. I search about this one and apparently there are no other workaround. I'm still thinking if I want to use a library again on other projects because I can't really control them compared to just using my own code. But hey, that you mentioned this one since I misspelled my prefers-reduced-motion to prefers-reduce-motion !!
  • Lastly for the content, like what I mentioned, can't control the library since it uses javascript to show those animations, but in terms of seo or using assistive-tech, the contents are still there, just hidden visually. Haven't added a noscript text on this one as well since I don't have any good reason to actually first think of user disabling their javascript except for like pop-ups or ads right. But thanks for this bit as well..

  Thanks again Vanza!

  1

  

  Vanza Setia•27,775

  @vanzasetiaPosted over 2 years ago

  @pikapikamart You're welcome! Glad to help! 😀

  About disabling JavaScript, usually, the users that disable JavaScript are the users that don't want to be tracked. For example, whenever I need to visit a site that has a lot of trackers or ads like the news sites, I like to disable JavaScript. That way, I don't need to see any annoying ads pop-up.

  In this case, of course, I have no reason to see your site without JavaScript. It's just something that I want to mention. 🙂

  2
• 

  T

  Matt Studdert•13,611

  @mattstuddertPosted over 2 years ago

  Great to see you post a solution for your first premium challenge, Raymart! ✨

  Your project looks fantastic, and I love the extra details like the animations and the sticky header. Great to see you add the prefers-reduce-motion query as well for those who prefer not to see animations.

  I'd recommend reviewing your implementation of the "Create plan" page with keyboard navigation in mind. I couldn't choose individual options as it always focused on the group when tabbing through the content. I could use the arrow keys, but I'd expect to be able to tab through the options easily and select one using space or enter, which I wasn't able to do.

  I'd recommend adding the name attribute to your radio inputs as well so that they're grouped appropriately.

  I hope that helps. Keep up the excellent work! 👍

  Marked as helpful

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @mattstuddert Hi Matt! Glad that you liked the site!

  For the create plan page, I don't know about this one since the groupings, those options bellow the buttons are set of radios and the expected navigation on them will be to use arrow keys since you can only select one.

  For the name attribute, glad that you pointed that one out Matt! It is my idea to not add the name attribute at first because I wasn't going to use the native form to destructure the value for each of the radio-group since I was using a controlled radio and changing the state for every change of radio hence adding the onChange for each one and also as long as radio inputs are group by a container, they will function the same with using keyboards and screen-reader as I tested with nvda but not sure with talkback since talkback don't have support for telling where a radio is group, it's really weird.

  Thanks again on this one Matt, going to add the name since it is the proper way!

  0
• 

  T

  Chamu•13,130

  @ChamuMutezvaPosted over 2 years ago

  Amazing work Raymart, well done. It maybe an issue with my cellphone, there is a section which is remaining blank when I scroll - will post you the images on slack

  Marked as helpful

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @ChamuMutezva Hi Chamu! Thanks again for mentioning about it and sending it in slack. The issue should be fixed by now, it was because of the whileInView property for a parent container from framer.

  I instead just added a custom hook to check for visibility so that it won't have to rely on that property and it will let the child component to animate if they are in view now. Thanks again!!

  1
• 

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

  Hi Raymart, you did a great job!

  I assign a descriptive name as if it were a class together with the tag that I am going to stylize, for example for a container div I call it DivContainer. Using Styled at the beginning I find it superfluous, once you see the tag followed by the pseudo description you can understand that it is nothing more than a styled component :)

  What do you think about it?

  Hope this help and happy coding!

  Marked as helpful

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @denielden Hey Deniel!

  I also make use of that convention as well, like treating those container just like that I would do with scss using bem convention and I find that at first my end naming convention but then since I was using styled, I could just create any naming right and I ended up making my naming to verbose :>>>

  Do you follow a convention when using styled-component? Do you have like a resource about that? Perhaps a link on what you follow?

  Trying to see how others go with it and maybe adapting one for myself. Thanks again!

  2

  

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

  @pikapikamart I don't use any conventions other than trying to make it as clear as possible that it's a stylized component to anyone reading the code.

  You can see an example in the code of this my solution

  0

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @denielden I see, so you like generalized the naming of the styled for each component. For example, you don't name the styled as the same name of the component since it will be only used on that one.

  I saw like ContainerDiv Table they are generalized since they will be used by the component itself and doesn't need extra naming.

  Thank you again on this!

  2
• 

  Danilo Blas•6,300

  @Sdann26Posted over 2 years ago

  You have an incredible result, congratulations.

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @Sdann26 Thank you Danilo!

  0
• 

  Hayatunnabi Nabil•130

  @imhayatunnabiPosted over 2 years ago

  nice

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @imhayatunnabi Thank youu!!

  0
• 

  Fayaz•380

  @fayax555Posted over 2 years ago

  You could prefix with an S instead of Styled. So StyledNavbar will become SNavbar.

  1

  ---

  

  Raymart Pamplona•16,100

  @pikapikamartPosted over 2 years ago

  @fayax555 I was thinking about that and I did at first as well, but I don't know why I don't like seeing 2 capital letter side-by-side like SN from SNavbar hahahaha. But hey, thank you for this!

  0