feat: add collapsible sections to the package page [part 1] #503
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
@rshigg this needs to work on hard reload, as well as remember the state of the sections, or we'll have layout shift when loading. ideally if the collapsed/expanded state of each section can be controlled via css, rather than the implicit behaviour of |
| class="group text-xs text-fg-subtle uppercase tracking-wider flex items-center gap-2" | ||
| <section | ||
| :id="id" | ||
| :aria-labelledby="id + '-title'" |
There was a problem hiding this comment.
| :aria-labelledby="id + '-title'" |
You shouldn’t make this a region landmark. Named sections, which become region landmarks, should be used sparingly and not as a default for any component. See #384 for more detailed reasoning.
There was a problem hiding this comment.
Great callout I'll remove the label
| <details open class="col-span-full row-start-1 open:pb-2"> | ||
| <summary |
There was a problem hiding this comment.
I’m a bit wary of using <details>/<summary> elements for sections of a page. Namely because nesting a heading within a <summary> element is awkward for screen readers. My rule of thumb is that if you need a heading, don’t use <details>/<summary> and instead make your own collapsible sections.
For example, when traversing a page, if a (macOS) VoiceOver user encounters one of these, it won’t immediately surface the heading (and it won’t be clear there is one). Conversely, if the user is navigating by heading (which is very common for SR users), it won’t surface the fact that it’s within a <summary> element and so it won’t be clear that the corresponding content is collapsed. NVDA + Firefox and iOS VoiceOver do a bit better in this regard, but NVDA and JAWS have the same issues when paired with Edge.
A better structure is to nest the expanding/collapsing <button> within the heading:
<section>
<h2><button type=button aria-expanded=true aria-controls=content-0>Expanded section</button></h2>
<div id=content-0>
<p>Collapsible content…</p>
</div>
</section>
<section>
<h2><button type=button aria-expanded=false aria-controls=content-1>Collapsed section</button></h2>
<div id=content-1 hidden=until-found>
<p>Collapsible content…</p>
</div>
</section>There was a problem hiding this comment.
I think getting rid of details/summary because of the headings might be throwing out the baby with the bath water.
Since all of these sections are in an aside we could simply get rid of the headings entirely. I like having the semantics of a disclosure widget for data that is considered "extra" to the main content.
What do you think?
There was a problem hiding this comment.
I think the headings are valuable for screen reader navigation, otherwise, you force screen reader users to find these sections themselves, so I wouldn’t remove them. And even within an <aside> element heading structure is still valuable.
Since the ancestor <aside> element is a descendant of an <article> element (a sectioning content element) it actually has no semantic value ARIA-wise (i.e. if it was labelled it would gain the role=complementary implicit ARIA semantics, see “3.5.10 aside (scoped to a sectioning content element)” in HTML-AAM. Firefox does something funky and might incorrectly surface it as role=complementary). Personally, I don’t think that the contents are tangential to the main content of the page though, so I wouldn’t label it (and I wouldn’t have even used an <aside> element in the first place—it’s doing no harm though).
There was a problem hiding this comment.
Gotcha, that all makes sense to me. Do you see value in the <article> ancestor here? we might want to remove all of these superfluous landmarks and just have everything inside a <main>
There was a problem hiding this comment.
On second thought it might be beneficial to wrap the README in an <article>
represents a self-contained composition in a document, page, application, or site, which is intended to be independently distributable or reusable
Seems relevant since a README is a distributable standalone file
There was a problem hiding this comment.
Good question. We don’t need to have the <article> for the page contents, but it doesn’t hurt. I think an <article> element could actually be better used to wrap the README, since that’s a standalone document (in which case, I would swap the <section> that currently wraps it with an <article>).
If the top-level <article> were removed it would cause the <aside> to gain the implicit complementary role (since <main> isn’t considered sectioning content), so in that case, I would actually change the <aside> to something else (a <div> element is fine IMO, since the sidebar here is more a presentational detail than a semantic one).
All of this is outside of the scope of the change this PR makes, so perhaps should be addressed separately. I might start a PR to wrap the README with an <article> instead of <section>.
There was a problem hiding this comment.
Great minds think alike! Yeah lets put the landmark changes in a separate PR
We can set the Regardless of how we achieve it my plan was to persist the open/closed states in local storage. |
|
ideally the code runs before the dom is rendered (ie a script in head) so that the user never sees even a flash of the wrong state... |
|
@danielroe I will revamp the markup based on @knowler's feedback and make sure to add toggling persistence without a flicker |
3 participants
Summary
Refactors the package page sidebar to use collapsible sections with a <details>/<summary> pattern.
Changes