Base apparel coming soon with CSS Grid and JS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Though I had to zoom out to see the desktop layout, are you using wide screen? Setting 1440px as a desktop version is a no, the 1440px on the design are just the sizes where the design is made and it is not related to the breakpoint, lowering it down would be really great. The mobile state looks fine however.

  Some other suggestions would be:

  • On desktop, the image should be occupying all height like on the design.
  • Avoid using height: 100vh on a large container like the body tag as this limits the element's height based on the remaining screen's height. Instead use min-height: 100vh, this takes full height but lets the element expand if needed.
  • Also don't add width: 100vw as this adds an extra horizontal scroll since this doesn't account the scrollbar's size.
  • Your main tag usage is incorrect, on this one, the .wrapper should be using the main instead of div. This way, your main content is wrapped inside a main-landmark element.
  • Also, you don't need:

  top
  transform
  

  To align the content, on the body tag add:

  align-items: center;
  display: flex;
  flex-direction: column;
  justify-content: center;
  min-height: 100vh;
  

  This way, it will be properly centered.

  • Website-logo should be present since it is one of the meaningful element on a page and use a proper alt on the img tag of it.
  • Like I said, main should be the `.
  • Would be really great for the input and button to be wrapped inside a form tag to make it more clearer what the component is.
  • Your input tag lacks an associated label tag on it. Since there are visible-label, the label would be a screen-reader only label, meaning it would make user of like sr-only class. The text-content should describe what the input needs like the value on the placeholder.
  • Right now, when an error appears, only sighted users will be guided that an error has shown because it is only made visually. Instead of this, do it this way, a pseudocode looks like:

  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.

  • If you have no reference about what I said, here is a simple accessible form that I have see the structuring and the attribute manipulations on the js. This might seem ambiguous right now, but making accessible form with these is our work^^.
  • The button needs to have type="submit", be explicit on what your button should act inside of a form.

  Aside from those, great work again on this one.

  Marked as helpful

  0