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.

• [x] 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
• [x] 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[bot]]

🦋 Changeset detected

Latest commit: e1b03a33ba3e4f6a4ef560539e46052e068ff309

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[bot]]

size-limit report 📦

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

[mperrotti]

The grid gaps are creating some spacing issues. Maybe instead of using grid-gap, we could just put padding on the elements in the grid.

If you feel grid-gap is a better way to go, we could conditionally change how many columns the title area spans depending on what elements are present.

Solution 1 that preserves grid-gap: Conditionally change how many columns or rows things span depending on what elements have been passed in. For example...

• If there's a leading visual and trailing action, keep it as-is
• If there's a leading visual and no trailing action, span from column 2 to 4
• If there's no leading visual or trailing action, span from column 1 to 4

Solution 2 that preserves grid-gap: Conditionally change the grid-template-columns and grid-template-areas values depending on what elements have been passed in.

These solutions could get complicated though...

We can pair some more if you want 🙁

[github-actions[bot]]

Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/3816#issuecomment-1823434546.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

[github-actions[bot]]

Uh oh! @mperrotti, the image you shared is missing helpful alt text. Check https://github.com/primer/react/pull/3816#issuecomment-1823434546.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

[broccolinisoup]

The grid gaps are creating some spacing issues.

@mperrotti I saw the gaps in the VRT tests but didn't realised it was that tricky 😭 Thanks for bringing it to my attention!

Maybe instead of using grid-gap, we could just put padding on the elements in the grid.

I am a bit hesitant about this approach; mainly because I think we want these gaps to be around the cells not around the items. Also I feel like your second suggestion aligns more with the overall direction in the implementation. We have grid template areas and since we don't know what sub components might be used in a given PageHeader usage, creating the areas dynamically makes more sense to me, though it is quite complex, I agree 😬 I pushed a super WIP draft PR to explore your second suggestion. https://github.com/primer/react/pull/3970 I'll tag you there when it is less WIP 😄

[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 https://github.com/primer/react/pull/4358