BASE APPAREL COMING SOON using CSS, Vanilla JavaScript, and BEM

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, great work on this one. Desktop layout looks really great, the site is responsive as well, though I early suggest to make the mobile breakpoint a bit sooner because at like point 770px, the image is too thin, so making that transition would prevent that or maybe letting the left-side resize as well. Mobile view looks great also.

  Some suggestions would be:

  • If you somehow inspect layout in dev tools at the bottom, the image is not occupying the full height of the screen thus its upper part is being hidden. So a quick fix, on your img tag, instead of using height: 100vh which is a bad styling, use min-height: 100vh so that it will scale properly.
  • If you use a primary header make sure that it is outside the main tag so that it will be treated as one of you primary landmark. For this one, it would be better to use :

  <header />
  <main />
  

  With the header being absolute on the top so that it won't affect the layout of the main.

  • Use only the website name as the website-logo alt value.
  • When using img tag, you don't need to add words that relates to "graphic" such as "logo" and others, since img is already an image so no need to describe it as one.
  • I would use a descriptive alt on the lady's image since if you look at it, the lady is somehow using clothing on the base-apparel, maybe she is showcasing the clothing, those ornaments.
  • On the h1, you don't have to use br to make each text wraps on another row. You can use max-width on the h1 itself and adjust it until each text are wrapped on their own row.
  • The button inside the form should be using type="submit". Remember that when a button is placed inside a form element, it defaults to type="submit". So imagine if you have a close-button inside the form without specifying type="button" click the close-button will submit the form. Be aware of this kind of scenarios.
  • The error-icon is only a decorative image. Decorative images should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if the image is using svg.
  • Right now, the error is only seen visually but not really linked with the input itself. For a proper error, message, it would look like this pseudocode:

  if ( input is wrong )
    input.setAttribute("aria-invalid", "true");
    input.setAttribute("aria-describedBy", id of the error-message);
  else 
    input.removeAttribute("aria-invalid");
    input.removeAttribute("aria-describedBy");
  

  The error-message element should have an id attribute which is referenced by the aria-describedBy attribute on the input element. By doing that, your user will know that the input is wrong because of aria-invalid and they will know what kind of error they made because of the aria-describedBy. Have a look at this accessible form snippet that I have On this one, you can see aria-live is used as well to inform the user right away on what is the status of the form submission.

  Aside from those, great job again on this one.

  Marked as helpful

  0

  ---

  

  Ken•935

  @kenreibmanPosted almost 3 years ago

  @pikamart Absolutely love the feedback! Thank you for taking the time to write this, it helps people implement better practice.

  I went ahead and changed some of that styling advice that you gave me, and I will be implementing the aria attributes to my JS later today.

  1
• 

  Jimmy Hoang•570

  @JimmyHoang296Posted almost 3 years ago

  hey man :)) really fun

  1

  ---

  

  Ken•935

  @kenreibmanPosted almost 3 years ago

  @GiaVoNgu thank you!

  0