-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix(a11y): use H2 header for dialog title
#7668
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
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
| .#{$ns}-heading { | ||
| @include overflow-ellipsis; | ||
| flex: 1 1 auto; | ||
| font-size: 14px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there's a special variable I can use here to reference the font size of h4 titles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could do something like this, but it might be worth just hard-coding 14px since we may not want this font-size to change if the default h6 font size changes.
| font-size: 14px; | |
| font-size: list.nth(map.get($headings, "h6"), 1); |
@use "sass:map";
@use "sass:list";
@import "../../common/typography";There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I thought as well...
fix(a11y): use `H2` header for dialog titleBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
gluxon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Let's wait for @ggdouglas on the final approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new prop, here's another proposal: Let's change the type of
string | React.Node. If it's a string then we render it inside an H2 header, else we use the provided node as is. That would keep a reasonable default while still allowing for further title customization.
WDYT @mm-wang? |
Changes proposed in this pull request:
Headings should reflect their visual level on the page.
<h6>isn't appropriate for a dialog title.Screenshot
I changed the font size to preserve the current appearance of the title.
Today:

With this change:
