Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make summary elements more accessible #530

Conversation

thewildmage
Copy link
Contributor

This PR fixes #504.

Checklist

  • I have signed the CLA
  • I have updated/added any relevant documentation

Description

What's the goal of this PR?

To make the usage of <summary> HTML element more accessible.

What changes did you make?

There were two main impediments to accessibility in the current implementation:

  1. There are <summary> elements that contain headings (i.e. <h1> through <h6>). This strips the headings of semantic meaning in some user setups. (This is the crux of Issue Summary elements shouldn't contain headings #504.)
  2. The <summary> elements have custom open/close arrows, which alter semantic meaning and usability in some user setups.

This PR fixes both of these by moving the headings outside of the <summary> elements and going back to using the default open/close arrows. Notably, the default open/close arrows were already in use for the "Recommended Settings" sections (under Container info), so this change also makes things more consistent overall.

What alternative solution should we consider, if any?

These changes are largely informed by "The details and summary elements, again" by Scott O'Hara, an accessibility specialist. The other suggestion there is to create fully custom open/close elements and not use <details> and <summary>. This alternative should be considered as it could make things even more usable and accessible than what this PR achieves. It'd be a bigger change though, which is why I've opted here for a more incremental improvement to start with.

Copy link
Member

@sudermanjr sudermanjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome sauce. thanks!

@sudermanjr sudermanjr enabled auto-merge (squash) October 2, 2022 04:09
@sudermanjr sudermanjr merged commit d0f9655 into FairwindsOps:master Oct 2, 2022
@thewildmage thewildmage deleted the thewildmage/no-headings-in-summary-elements branch October 2, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Summary elements shouldn't contain headings
2 participants