Skip to content

Conversation

@broccolinisoup
Copy link
Member

This PR changes the PageHeder.Title's default heading level to h2 as discussed here

Also added a feature story and docs to demonstrate the cases where components are used as a title instead of a typical text title and included a visually hidden title to set an example.

Closes https://github.com/github/primer/issues/1571

Screenshots

No visual changes.

Merge checklist

  • Added/updated documentation
  • Tested in Chrome

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented Feb 17, 2023

🦋 Changeset detected

Latest commit: 6ab9078

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@broccolinisoup broccolinisoup marked this pull request as ready for review February 17, 2023 06:24
@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 94.58 KB (0%)
dist/browser.umd.js 95.15 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-2905 February 17, 2023 06:29 Inactive
@primer primer bot temporarily deployed to github-pages February 17, 2023 06:31 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2905 February 17, 2023 06:31 Inactive
@TylerJDev
Copy link
Member

Slightly off-topic, but within the docs under the as prop, we state "header" as the default value. I wonder if we'd want
"div" here?

Copy link
Member

@TylerJDev TylerJDev left a comment

Choose a reason for hiding this comment

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

Overall looks good! I left some comments that might require some discussion, but other than that I think it looks good. Thanks for this too! 😁

<Breadcrumbs.Item href="#">PageHeader</Breadcrumbs.Item>
<Breadcrumbs.Item href="#">PageHeader.tsx</Breadcrumbs.Item>
</Breadcrumbs>
<h2 className="visually-hidden">Visually Hidden Title</h2>
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that in the preview for this example, the heading isn't actually hidden. Is this expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

😬 No it is NOT EXPECTED 😅 I'll fix it

@broccolinisoup
Copy link
Member Author

broccolinisoup commented Feb 22, 2023

Slightly off-topic, but within the docs under the as prop, we state "header" as the default value. I wonder if we'd want
"div" here?

Oh good catch! On the code it is div but apparently I got it wrong on the docs. I'll update it.

@github-actions github-actions bot temporarily deployed to storybook-preview-2905 February 22, 2023 05:35 Inactive
@primer primer bot temporarily deployed to github-pages February 22, 2023 05:35 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2905 February 22, 2023 05:36 Inactive
@broccolinisoup
Copy link
Member Author

@TylerJDev I pushed a commit to make the visually hidden titles visually hidden for real 😅 and fix the docs about the default as.

You can see in the storybook I used VisuallyHidden component whereas I added styles via sx prop on the primer.style docs. My motivation behind this was about the docs practicality. Primer.style docs examples should just work when people copy and paste the code snippets provided and given the fact that the VisuallyHidden component is an internal util component, I manually added its styles via sx prop. Let me know if you have any concern 🙌🏻

@TylerJDev
Copy link
Member

@broccolinisoup - Looks great 🤩. Thank you again!

@github-actions github-actions bot temporarily deployed to storybook-preview-2905 February 23, 2023 01:35 Inactive
@broccolinisoup broccolinisoup temporarily deployed to github-pages February 23, 2023 01:37 — with GitHub Actions Inactive
@broccolinisoup broccolinisoup added this pull request to the merge queue Feb 23, 2023
Merged via the queue into main with commit 6bf9e67 Feb 23, 2023
@broccolinisoup broccolinisoup deleted the pageheader-title-heading branch February 23, 2023 22:21
@primer-css primer-css mentioned this pull request Feb 23, 2023
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.

4 participants