Fix title and subtitle tag for Dialog2 and ConfirmationDialog #1754
Conversation
🦋 Changeset detectedLatest commit: 680361f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
siddharthkp
changed the title
siddharthkp
added
accessibility
patch release
size-limit report 📦
|
|
LGTM. Why the margin overrides though? |
| ${sx}; | ||
| ` | ||
|
|
||
| const Subtitle = styled.div<SxProp>` | ||
| const Subtitle = styled.h2<SxProp>` |
There was a problem hiding this comment.
Wondering if this makes semantic sense. Should it be a p instead?
There was a problem hiding this comment.
Following it from #1753, should be h2
There was a problem hiding this comment.
Hmm, I've been thinking about this and it's a good question. I naturally leaned towards <h2> but I'm not sure that's actually the right thing. Maybe @bolonio will have opinions here?
Sorry if that turns out to be the wrong tag for the description!
There was a problem hiding this comment.
First of all, we can agree that Dialog title headings should be <h1>. I would go with <h2> for the subtitle as well, since modal dialogs are considered areas that are distinct from their parent document, so they have their own heading level structure. I wouldn't say that using a <p> is wrong, since not having a <h2> heading is possible. I've been reading about it and I couldn't find anything about subtitle or <h2> in dialog (of course there are some information about the dialog title to be <h1>).
Related issues:
h1 and h2 have default margins in the browser's stylesheet, so we we override it with the margin we want. |
There was a problem hiding this comment.
Nice one @siddharthkp ⚡. Would be nice to have some test coverage on the heading roles, but we can follow-up on that later.
Fixes #1753
title should use h1 and subtitle should use h2 instead of div
Merge checklist