3-column-preview-card-component-main

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, really nice work on this one. The desktop layout looks really nice, for responsiveness, currently the site's content is being hidden when you go to 950px below because the layout is not responding well on those part. The mobile state looks great though.

  Martin Eliáš already really great feedback on this one, just going to add some suggestions on the site as well:

  • The .attribution tag which should use the footer tag should be placed outside the main tag so that it will be read as a primary landmark:

  <main />
  <footer />
  

  • If you zoom out of the page, the layout is not being centered vertically since margin alone is not consistent. For this, you could remove the margin from the .container selector and on the body tag, you could add these stylings:

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

  • You can remove the figure on this one and just the img tag itself. Normally, when using figure, there should be a visible text to be used as figcaption.
  • For each of the car-icons img tags, adding an extra aria-hidden="true" on it would be really nice so that they will be totally ignored by screen-readers.
  • Just to reiterate what Martin has said about h1 tags. Only have a single h1 on a site. It would be great to change those headings into something like h2.
  • On a site, always have a single h1. Since there are no visible text that are suitable to be h1, the h1 would be a screen-reader only heading. Meaning it will be hidden visually but present for screen-reader users. On this, the h1 would have like sr-only class and the text-content should describe what is the main-content is all about. The h1 could be placed as the first text inside the main. Have a look at Grace's solution on this same challenge. Inspect the layout and see the usage of h1 as well the stylings applied to it.
  • Those learn-more are better using a tag rather than button. On a real site, this component, the learn-more would be a page-link where user can "learn more" about a specific car.
  • The .attribution, you can remove the position: absolute on it since it just goes in the screen when inspecting the layout from dev tools at the bottom.
  • Lastly, just making the site as responsive as you can:>

  Aside from those, great job again on this one.

  Marked as helpful

  0
• 

  Martin Eliáš•890

  @martinelias1312Posted almost 3 years ago

  Hello @Wrath-code, good job with relative units, variables, classes.

  I found one thing and it is that your <div class="container"> is wrapping all your content, it can be fixed by this end tag </div> should be upper than your </main> end tag which also missing "/" .

  And your signature <div class="attribution">, should be wrapped in <footer> element for more semantic HTML layout and better accessibility.

  One more suggestion is that only one <h1> element should be used, because it is beneficial for screen reader users. Look at this article

  Marked as helpful

  0