Mobile-first workflow, CSS grid, Semantic HTML

Community feedback

• 

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @sivakotari 👋🏻

  I have some suggestions on how to fix the accessibility, HTML and some other minor issues.

  • To fix the accessibility issues, you need to change <div class="attribution">...</div> to <footer class="attribution">...</footer>. This will fix all of your accessibility issues. Don't forget to generate a new repot once you fix the issue
  • I'd also suggest replacing <article> with <div> for a couple of reasons. First, in every article you have to have a heading such as h1-h6, which is irrelevant is this case. Next, <article> is designated for a newspaper article, or a blog entry, etc.
  • And this is a more personal suggestion, but I'd add the same amount of margin to the bottom of .app-container as its top margin, so something like this margin: 6.25rem auto 6.25rem auto;

  Other than that, you did a superb job, it looks really close to the design. I hope this was helpful 👨🏻‍💻 Cheers 👾

  Marked as helpful

  1

  ---

  

  SivaKotari•135

  @sivakotariPosted about 3 years ago

  @kens-visuals, Thanks you for the feedback! working on accessibility and style issues.

  0

  

  SivaKotari•135

  @sivakotariPosted about 3 years ago

  @kens-visuals updated!

  0

  

  P

  Ken•4,915

  @kens-visualsPosted about 3 years ago

  Hey @sivakotari,

  I'm glad you did, but unfortunately I don't see any changes. Did you push the updated version to your GitHub repo?

  And one more thing before you check that or try pushing it again, you can also remove unused srcset="" from img tag, so there will be no HTML issues at all 🙃

  0

  

  SivaKotari•135

  @sivakotariPosted about 3 years ago

  Hey @kens-visuals , So strange, I could see the latest in Git! BTW, fixed srcset="" accessibility bug. Thank you

  0