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.

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

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

Merged
merged 25 commits into from
May 29, 2024

Conversation

broccolinisoup
Copy link
Member

@broccolinisoup broccolinisoup commented Mar 6, 2024

Copied from #3816 (due to merge conflicts)

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.

Copy link

changeset-bot bot commented Mar 6, 2024

🦋 Changeset detected

Latest commit: 7bb732d

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

@broccolinisoup broccolinisoup changed the title Update layout styles so that all interactive elements come after the title PageHeader: Address dom order issues (screen reader experience feedback from sign-off) Mar 6, 2024
Copy link
Contributor

github-actions bot commented Mar 6, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 89.34 KB (0%)
packages/react/dist/browser.umd.js 89.68 KB (0%)

@github-actions github-actions bot temporarily deployed to storybook-preview-4358 March 6, 2024 01:27 Inactive
@broccolinisoup broccolinisoup added this pull request to the merge queue May 29, 2024
Merged via the queue into main with commit e34e4b2 May 29, 2024
30 checks passed
@broccolinisoup broccolinisoup deleted the pageheader-sign-off-review-address branch May 29, 2024 08:55
@primer primer bot mentioned this pull request May 29, 2024
khiga8 pushed a commit that referenced this pull request May 31, 2024
…ck from sign-off) (#4358)

* Update layout styles so that all interactive elements come after the title

* Add PageHeder.Breadcrumbs sub component

* remove shapshot check on jest

* test(vrt): update snapshots

* test(vrt): update snapshots

* make the interactive element in the heading check a warning not error

* add pageheader story for screen reader tes

* add links

* add onclick events to the buttons

* add a placeholder for file content example

* test(vrt): update snapshots

* elements losing context when viewport is narrrow - keep the same text for buttons in all viewports

* fix linting

* test(vrt): update snapshots

* hide the overflow menu in narrow since we show it on the context area

* accomodate different format sx prop for font size

* when no fontsize is specified css var returns as an empty string so need ternary instead of Nullish coalescing

* temporary fix on navlist

---------

Co-authored-by: broccolinisoup <broccolinisoup@users.noreply.github.com>
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.

None yet

3 participants