News homepage site with SCSS

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted over 1 year ago

  Hi

  I see issues on this with your html and with styling approach (mainly relating to that sticky header)

  1. You should not have two sets of markup for the mobile and larger screen navs. You should have one set of meaningful html only in a header and use css to change the styling when necessary (mobile as the default). The mobile nav trigger button must sit inside the nav element and come before the list of content it is triggering in the DOM and must have an aria-expanded attribute. The close Button would usually go after the list in terms of dom order and will also need an aria-expanded attribute (note it is site nav not page nav too)
  2. The HTML on this challenge requires some visually hidden headings to make the content make sense. Namely it needs a h1 page title, a h2 for "featured article" and a h2 for the list of articles at the bottom eg "popular articles". Then all article names would be h3s. "The bright future..." Article heading makes no sense as a h1 for the page. It could be a h2 if you wanted to skip having a "featured article" heading.
  3. Media queries should always be defined in rem or em. And 1440px wide is very very late to make that switch
  4. In strictest html sense, the links to the articles should go inside the heading elements, not wrap them. It makes little difference in this case but would matter a lot if it was a button wrapping so is good to get out of that habit now
  5. The last section in the design is clearly an ordered list, not an unordered list. Once that's restored you can probably aria-hide the articles__popular-number elements
  6. H4 is definitely the wrong heading level for those popular articles. That would imply they are all part of the "Is VC Funding Drying Up?" content as that is the h3 they would currently "belong to"
  7. I would not recommend using a sticky/fixed nav on this. It is currently taking up a lot of screen real estate, especially when viewed in landscape orientation or when text size is increased.
  8. On a related note, it is unusual to be able to scroll the page content when the nav is open and an overlay present. Users need to be able to scroll inside the nav (eg when the screen is short or their text size enlarged) but this is not possible at the moment. What you would need to do is something like track scroll position on menu open with js, disable scroll on the body with css, allow scroll with overflow inside the nav on mobile, return user to their original scroll position on close. That's a load of work that would be much simpler if you didn't have the fixed/sticky nav
  9. On the question of how to deal with huge screens, I would always limit content width. You can use clamp and make the font sizes a little bigger but only up to a point. And then you start creating barriers in your attempt to be helpful - someone with an astigmatism or long sighted people would find huge text and other content just as difficult to read as many people do with tiny text

  Marked as helpful

  1

  ---

  

  Max Ruuen•290

  @ruuenPosted over 1 year ago

  @grace-snow Thanks again for your amazing feedback Grace, learned a ton of new things while implementing these fixes and have picked up some good screen reader usage skills in the process for my future projects & accessibility planning.

  I've corrected all items under a dev branch which is deployed to a separate Netlify site

  The dev branch with changes is on Github.

  0
• 

  T

  Grace•31,270

  @grace-snowPosted over 1 year ago

  Looks good!

  A few issues still

  1. The aria-expanded value on the nav buttons isn't changing on click - definitely fix that.
  2. The aria-controls is pointing to an ID on the wrong element. It should be pointing to an ID on the actual content it is toggling (the UL)
  3. You don't need a visually hidden heading on the new articles. It already has a visible heading and that's fine! That shouldn't be aria-hidden or only in a span. Just have the one heading, no need to over-complicate it
  4. On list elements when you remove list-styling in CSS some user agents stop announcing them as lists any more. An easy way to fix this in the html is to add role="list" to the ul/ol elements and role="listitem" to each li. It doesn't do any harm for agents that have those semantics anyway, but it really helps on the agents that removed the semantics

  0