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

Restricted allowed heading for popover slot #673

Merged
merged 2 commits into from
Jul 1, 2021
Merged

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Jun 30, 2021

Closes #491

What

This PR restricts the allowed tags for popover component heading slot to heading tags. This is the last of components and slots needing tag restrictions.

Notes

Usually, I don't think it's great to have a default for heading since the appropriate heading level tends to be context-dependent. However, I think Popover might be a special case, similar to a modal dialog. Popover appears temporarily on mousehover and doesn't fall within a normal page structure.

There seems to be a lot of debate around what the most appropriate heading level for a modal dialog is but nothing prescriptive.
There's a comment in w3 aria-practices discussion:

It is helpful if there is a consistent approach across dialogs in an application.
There is not necessarily a need for the APG to prescribe a specific approach.

I think we can think of Popover similarly where maybe it's more important to have consistency across Popover. For now, I will leave the :h4 default though we may want to revisit this in the future. Additionally, in dotcom, Popover is actually not accessible at all for keyboard-only users so that is a whole other can of worms we need to address...

@khiga8 khiga8 requested a review from a team June 30, 2021 20:33
@vercel
Copy link

vercel bot commented Jun 30, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/view-components/E9VosdJaiCETqLmHRNaXbiTuocYM
✅ Preview: https://view-components-git-kh-headingpopover-primer.vercel.app

@khiga8 khiga8 force-pushed the kh-heading_popover branch from 121f89b to ea7656c Compare June 30, 2021 20:34
@vercel vercel bot temporarily deployed to Preview June 30, 2021 20:34 Inactive
@vercel vercel bot temporarily deployed to Preview June 30, 2021 22:32 Inactive
@joelhawksley joelhawksley merged commit 3bc563a into main Jul 1, 2021
@joelhawksley joelhawksley deleted the kh-heading_popover branch July 1, 2021 14:06
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.

Consolidate how we set default tag and allowed tags for accurate documentation
2 participants