PageHeader: Address dom order issues (screen reader experience feedback from sign-off)

[broccolinisoup]

As a part of addressing accessibility sign-off review comments https://github.com/github/primer/issues/1115#issuecomment-1499501472, this PR updates the stories to ensure the order of the elements are the following;

1. PageHeader.Title
2. ContextArea (all elements)
3. PageHeader.LeadingAction
4. PageHeader.TrailingAction
5. PageHeader.Actions

instead of

1. ContextArea (all elements)
2. PageHeader.LeadingAction
3. PageHeader.Title
4. PageHeader.TrailingAction
5. PageHeader.Actions

The motivation behind is to make sure the actions on the page that are displayed above or before the main heading (Context area actions and leading action) should come after the main title (PageHeader.Title). With this way, we make sure screen reader users who navigate through heading menus don't miss any actions.

While we updated the dom order in the stories and this is how we will recommend to order the elements, we made sure everything, visually stays the same. (Leveraging CSS grid layout)

sign-off review comment reference

Changelog

New

Changed

• Layout of the Root element from flex to grid
• The order of PageHeader's sub components in the stories.

Removed

Rollout strategy

This is technically a breaking change but PageHeader is still a draft component, so we are good to realise this as a patch. There is one instance at github/github to be updated and I'll list the commit here to be included in the release PR.

• Patch release
• Minor release
• Major release; if selected, include a written rollout or migration plan

Testing & Reviewing

Merge checklist

• Added/updated tests
• Added/updated documentation (After I get 👍🏻 on the solution and the direction, I'll update the docs)
• Added/updated previews (Storybook)
• Changes are SSR compatible
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

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]

🦋 Changeset detected

Latest commit: e1b03a3

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	Minor

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	113.24 KB (0%)
dist/browser.umd.js	113.88 KB (0%)

[mperrotti]

Approved assuming we'll have a follow-up PR to get rid of the hack where we render a breadcrumb in the LeadingAction slot.

[mperrotti]

@broccolinisoup - why does this have the "skip changeset" label? Wouldn't we want to cut a release with these changes?

[broccolinisoup]

@broccolinisoup - why does this have the "skip changeset" label? Wouldn't we want to cut a release with these changes?

Yes, I usually add skip label to the big PRs in the beginning to avoid the CI job failing. After changes are clear, I write a changeset that reflect the changes. Sorry I didn't communicate about it. Considering this is approved, I'll add a changeset now. For your info, because it requires updates at dotcom, I'll hold off merging the PR until I am back from holidays (Jan 11th)

[broccolinisoup]

closing this due to snapshot merge conflicts and opened another PR #4358