Skip to content

Fix title and subtitle tag for Dialog2 and ConfirmationDialog #1754

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Fix title and subtitle tag for Dialog2 and ConfirmationDialog #1754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75665d9
use h1 + h2 for Dialogs
siddharthkp Dec 20, 2021
680361f
add changeset
siddharthkp Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dialogs-title-tag.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Fix title and subtitle tag for Dialog2 and ConfirmationDialog
3 changes: 2 additions & 1 deletion src/Dialog/ConfirmationDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,12 @@ const StyledConfirmationHeader = styled.header`
display: flex;
flex-direction: row;
`
const StyledTitle = styled(Box)`
const StyledTitle = styled(Box).attrs({as: 'h1'})`
font-size: ${get('fontSizes.3')};
font-weight: ${get('fontWeights.bold')};
padding: 6px ${get('space.2')};
flex-grow: 1;
margin: 0; /* override default margin */
`
const ConfirmationHeader: React.FC<DialogHeaderProps> = ({title, onClose, dialogLabelId}) => {
const onCloseClick = useCallback(() => {
Expand Down
9 changes: 5 additions & 4 deletions src/Dialog/Dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,17 +314,18 @@ const Header = styled.div.attrs<SxProp>({as: 'header'})`
flex-shrink: 0;
`

const Title = styled.div<SxProp>`
const Title = styled.h1<SxProp>`
font-size: ${get('fontSizes.1')};
font-weight: ${get('fontWeights.bold')};

margin: 0; /* override default margin */
${sx};
`

const Subtitle = styled.div<SxProp>`
const Subtitle = styled.h2<SxProp>`
Copy link
Contributor

@rezrah rezrah Dec 21, 2021

Choose a reason for hiding this comment

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

Wondering if this makes semantic sense. Should it be a p instead?

Copy link
Member Author

@siddharthkp siddharthkp Dec 21, 2021

Choose a reason for hiding this comment

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

Following it from #1753, should be h2

Copy link
Contributor

@owenniblock owenniblock Dec 21, 2021

Choose a reason for hiding this comment

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

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!

Copy link

@bolonio bolonio Dec 21, 2021

Choose a reason for hiding this comment

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

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:

font-size: ${get('fontSizes.0')};
margin-top: ${get('space.1')};
color: ${get('colors.fg.muted')};
margin: 0; /* override default margin */
margin-top: ${get('space.1')};

${sx};
`
Expand Down