mdn / developer-portal

The code that generates the MDN Web Docs Developer Portal.
Mozilla Public License 2.0
61 stars 38 forks source link

1719: Improve markup of Featured Items component for improved a11y #1737

Closed stevejalim closed 3 years ago

stevejalim commented 3 years ago

This changeset removes all divs from the markup in favour of unordered lists. Now have a list of sublists, which represents both how the content is structured in terms of heirarchy and still flows appropriately.

(Related issue #1719)

How to test

schalkneethling commented 3 years ago

On https://developer-portal.stage.mdn.mozit.cloud/topics/firefox-mobile/ I notice that the featured items is marked up as below:

<ul class="featured-items-wrapper">
    <li>
      <ul class="mzp-l-card-third">
        <li class="mzp-c-card mzp-c-card-small mzp-has-aspect-16-9"></li>
        <li class="mzp-c-card mzp-c-card-small mzp-has-aspect-16-9"></li>
        <li class="mzp-c-card mzp-c-card-small mzp-has-aspect-16-9"></li>
      </ul>
  </li>
</ul>

We have a list of 1 item that then contains another list of three items. I wonder, could we flatten it to just:

<ul class="featured-items-wrapper">
  <li class="mzp-c-card mzp-c-card-small mzp-has-aspect-16-9"></li>
  <li class="mzp-c-card mzp-c-card-small mzp-has-aspect-16-9"></li>
  <li class="mzp-c-card mzp-c-card-small mzp-has-aspect-16-9"></li>
</ul>

I edited the HTML in the browser and visually it would be fine, not sure if it is problematic from a template logic perspective though.

@stevejalim Thoughts?

stevejalim commented 3 years ago

@schalkneethling Thanks for querying that - i get what you mean. The featured-items component can render 2 to 7 items depending on what's configured and splits them according to some opinionated logic.

eg: for five items, we will have

[   1   ]  [   2   ]
[ 3 ] [ 4 ] [ 5 ] 

which is rendered as a list of two rows, and each row is itself a list of items. I thought that seemed fairly logical/sensible in markup terms, but perhaps not.

What you're spotting here is where there is just one row of items to render, yet we still ahve that outer wrapping ul (ie a li for just one row, with its own ul and lis inside)

I think i could clean up the logic in the template so that if there is only one row to render, we don't need to nest a ul inside a ul, though it will make the template logic a little busier.

Happy to try, particularly if screen reader/a11y support is hampered in this case.

Question: are you happy with the main list with sublists approach when there are two rows of featured items, though? eg on https://developer-portal.stage.mdn.mozit.cloud/topics/rust/ (which I've added more items to)

schalkneethling commented 3 years ago

@stevejalim Makes sense, let's not overcomplicate this.