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.

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

Jump to bottom

fix: Use semantic Heading for the title of a Dialog #1123

Merged
merged 8 commits into from
Mar 14, 2024
Merged

fix: Use semantic Heading for the title of a Dialog #1123

merged 8 commits into from
Mar 14, 2024

Conversation

dlnr
Copy link
Contributor

@dlnr dlnr commented Mar 8, 2024 •
edited
Loading

Styling is supposed to be level 4 but what about the level? H2?

@dlnr dlnr requested a review from VincentSmedinga March 8, 2024 15:21
@alimpens
Copy link
Contributor

Styling is supposed to be level 4 but what about the level? H2?

This is actually a fairly complex topic 😅 Here's an informative thread in the Material UI repo on it.

An argument can be made that a dialog is a document that lives outside the main document, and as such needs an h1. I think that makes sense, but there are different views on this subject.

@dlnr
Copy link
Contributor Author

dlnr commented Mar 12, 2024

Changed the heading level to 1, I think it makes sense.

@github-actions github-actions bot temporarily deployed to demo-DES-603-Dialog-heading March 12, 2024 09:36 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-603-Dialog-heading March 12, 2024 14:31 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-603-Dialog-heading March 12, 2024 15:15 Destroyed
alimpens
alimpens reviewed Mar 13, 2024
View reviewed changes
packages/css/src/components/dialog/dialog.scss Outdated Show resolved Hide resolved
packages/react/src/Dialog/Dialog.tsx Outdated Show resolved Hide resolved
packages/react/src/Dialog/Dialog.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to demo-DES-603-Dialog-heading March 14, 2024 09:29 Destroyed
@dlnr dlnr requested a review from alimpens March 14, 2024 09:45
@github-actions github-actions bot temporarily deployed to demo-DES-603-Dialog-heading March 14, 2024 09:45 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-603-Dialog-heading March 14, 2024 11:51 Destroyed
VincentSmedinga
VincentSmedinga approved these changes Mar 14, 2024
View reviewed changes
packages/react/src/Dialog/Dialog.tsx Outdated Show resolved Hide resolved
@VincentSmedinga VincentSmedinga changed the title fix: Dialog Heading component fix: Use semantic Heading for the title of a Dialog Mar 14, 2024
@VincentSmedinga VincentSmedinga merged commit 0493fd8 into develop Mar 14, 2024
5 checks passed
@VincentSmedinga VincentSmedinga deleted the DES-603-Dialog-heading branch March 14, 2024 12:07
@github-actions github-actions bot mentioned this pull request Mar 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VincentSmedinga VincentSmedinga VincentSmedinga approved these changes

@alimpens alimpens Awaiting requested review from alimpens

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dlnr @alimpens @VincentSmedinga