add SkipToContent button to SubdomainNavBar #503

joseph-lozano · 2023-11-22T15:03:04Z

Summary

Adds a SkipToContent button to the SubdomainNavBar

List of notable changes:

Adds a SkipToContent button to the SubdomainNavBar

What should reviewers focus on?

Does the button work?
Does it properly hide itself when not in focus?
Does it jump to the content of the page.

Steps to test:

You can test this component on any of the SubdomainNavBar stories, e.g. https://primer-6e6587907b-26139705.drafts.github.io/storybook/?path=/story/components-subdomainnavbar--playground

Supporting resources (related issues, external links, etc):

Solves #404

Contributor checklist:

All new and existing CI checks pass
Tests prove that the feature works and covers both happy and unhappy paths
Any drop in coverage, breaking changes or regressions have been documented above
New visual snapshots have been generated / updated for any UI changes
All developer debugging and non-functional logging has been removed
Related issues have been referenced in the PR description

Reviewer checklist:

Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
Check that tests prove the feature works and covers both happy and unhappy paths
Check that there aren't other open Pull Requests for the same update/change

Screenshots:

Please try to provide before and after screenshots or videos

Before	After

changeset-bot · 2023-11-22T15:03:08Z

🦋 Changeset detected

Latest commit: 3e86b7d

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

This PR includes changesets to release 6 packages

Name	Type
@primer/react-brand	Minor
@primer/brand-primitives	Minor
@primer/brand-e2e	Minor
@primer/brand-fonts	Minor
@primer/brand-config	Minor
@primer/brand-storybook	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 · 2023-11-22T15:05:53Z

🟢 No design token changes found

github-actions · 2023-11-22T15:10:17Z

⚠️ Visual differences found

Our visual comparison tests found UI differences.

Please review the differences by using the test artifacts to ensure that the changes were intentional.

Artifacts can be downloaded and reviewed locally.

Download links are available at the bottom of the workflow summary screen.

Example:

If the changes are expected, please run npm run test:visual:update-snapshots to replace the previous fixtures.

Review visual differences

packages/react/src/SubdomainNavBar/SubdomainNavBar.module.css

rezrah · 2023-11-30T13:15:01Z

packages/react/src/SubdomainNavBar/SubdomainNavBar.module.css

+  width: 1px;
+  height: 1px;
+  padding: 0;
+  overflow: hidden;
+  clip: rect(1px, 1px, 1px, 1px);
+  border: 0;


This is just visually hiding it, right? If so, could this be done programmatically by adding the global visually-hidden class?

rezrah · 2023-11-30T13:16:50Z

packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx

+      <Button
+        as="a"
+        href="#start-of-content"
+        variant="primary"
+        className={clsx(styles['SubdomainNavBar-skip-to-content'])}
+      >
+        Skip to content
+      </Button>


@auareyou Checking that you've seen the proposed design here? I'm not sure the button styling works, and that we need something a bit more custom. The button feels too big IMO. What do you think? Happy to take your steer here.

cc. @nsolerieu @natalie-iv

Discussed at maintainer sync. We'll move forward with this design as an interim-step, with @auareyou exploring more integrated presentation down the line.

packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx

.changeset/eleven-dogs-flow.md

nsolerieu · 2023-12-04T19:28:02Z

@joseph-lozano I'd suggest use banner stretching under the header to avoid having the button awkwardly overlap the header UI like this:

Since this would be a custom additions we could consider using our button component and stretch it full width (like Linear does). Since our CTA component is very chunky it would look like this - ideally we'd want to remove the border radius for it too look more like the banner above.

Happy to see/review whatever is feasible here

packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx

rezrah · 2023-12-07T09:33:09Z

packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx

+        onFocus={() => setStartOfContentButtonFocused(true)}
+        onBlur={() => setStartOfContentButtonFocused(false)}


Possible to extract these functions outside of the return statement please? Potentially wrapping in a useCallback? Micro-perf optimisation to avoid creating a new function each time. I missed this last time, sorry.

rezrah

LGTM. Added some more minor feedback, otherwise good to go

Nice work @joseph-lozano ✨

joseph-lozano · 2023-12-13T17:20:04Z

After chatting at the Accessiblity Eng+ Office Hours, it sounds like we should first look for a <main> element before falling back to creating an empty div for the start-of-content.

Additionally, if we create the start-of-content empty div, we should make sure to give it tabindex="-1".

https://github.com/github/accessibility/issues/5240

joseph-lozano · 2023-12-14T17:23:02Z

packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx

+  useEffect(() => {
+    const main = document.getElementsByTagName('main')[0] as HTMLElement | undefined
+    if (!main) return
+    if (!main.id && startOfContentID) main.id = startOfContentID
+    setMainTag(main)
+  }, [setMainTag, startOfContentID])


@rezrah this seems... somehow wrong, but unsure how else I might satisfy the a11y team's recommendation. Do you have any thoughts?

I wouldn't recommend storing the element in React state, if that's what you're asking?That's really heavy and refs are better for this IMO.

This is just pseudocode so you'll need to try it, but can you do something like:

const mainElRef = useRef<HTMLElement | null>(null); useEffect(() => { const main = document.getElementsByTagName('main')[0] as HTMLElement | undefined if(el and otherStuff) { mainElRef.current = el; } }, []);

@rezrah my man, I think this is GTG ☺️ , except these two testing stories related to this update are giving AXE errors in CI! Would you know why it is failing? My AXE tooling is showing 0 issues even on strict. And running test:axe doesn't work on Codespaces, so I'll have to try this on my local.

Do we even need them?

stamat · 2024-03-05T15:47:11Z

@rezrah thanks for helping me sort out running the test:axe. It turned out that it was missing a directory so mkdir apps/docs/public/static which was breaking the run of npm run build:prod --workspace=apps/storybook. So adding the empty dir like you said made it run.

But then this wasn't what I needed, what I needed was to get that storybook-static directory. So you helped me by mentioning npx storybook build so I did this:

cd apps/storybook
npm i # which I think doesn't do anything cause node_modules are in the root directory
npx storybook build

This gave me the storybook-static directory with the stories.json that is needed for the packages/e2e/scripts/playwright/axe-clean.spec.ts to run when called by npm run test:axe --workspace=packages/e2e since this was the CI that was failing and since we don't list all of the violations, I couldn't know this before I got this working locally. I offer to try to write the code for listing the violations, shouldn't be that hard ☺️, so we can see directly from the CI what AXE is telling us. 🪓

So I curated the stories.json to run the two stories in question and found out what it was complaining about, which is this 👇

These stories include <main> tags which automatically have the role="main" and AXE doesn't allow us to have any other role for the <main> tag. But also we have a default Storybook div role="main" which was killing our buzz. 👇

I wonder if we can load the desired story independently of the story chooser. I think we should be able to do so. I'll try to update the code. Here is the suggested sketch: #546

But as the instant remedy I added these two stories in question to the ignore list. Since they are just for testing the ability to scroll to <main>.

P.S. The reason I needed to curate the stories.json is because I was getting an AXE violation for this 👇 😆 It's a banner telling us to update storybook. I think loading the stories independently of the chooser will ignore this violation.

joseph-lozano temporarily deployed to github-pages November 22, 2023 15:13 — with GitHub Actions Inactive

joseph-lozano marked this pull request as ready for review November 22, 2023 21:11

joseph-lozano requested a review from rezrah November 22, 2023 21:11

joseph-lozano temporarily deployed to github-pages November 22, 2023 21:20 — with GitHub Actions Inactive

joseph-lozano linked an issue Nov 27, 2023 that may be closed by this pull request

[Accessibility] Add a "Skip To Content" button to SubdomainNavBar #404

Closed

joseph-lozano self-assigned this Nov 29, 2023

rezrah reviewed Nov 30, 2023

View reviewed changes

packages/react/src/SubdomainNavBar/SubdomainNavBar.module.css Outdated Show resolved Hide resolved

rezrah reviewed Nov 30, 2023

View reviewed changes

packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx Outdated Show resolved Hide resolved

joseph-lozano temporarily deployed to github-pages November 30, 2023 14:48 — with GitHub Actions Inactive

joseph-lozano temporarily deployed to github-pages November 30, 2023 14:56 — with GitHub Actions Inactive

joseph-lozano requested a review from rezrah December 4, 2023 17:20

joseph-lozano commented Dec 4, 2023

View reviewed changes

.changeset/eleven-dogs-flow.md Show resolved Hide resolved

rezrah reviewed Dec 7, 2023

View reviewed changes

packages/react/src/SubdomainNavBar/SubdomainNavBar.tsx Show resolved Hide resolved

rezrah reviewed Dec 7, 2023

View reviewed changes

rezrah approved these changes Dec 7, 2023

View reviewed changes

joseph-lozano marked this pull request as draft December 13, 2023 17:20

Joseph Lozano added 5 commits December 14, 2023 10:46

add SkipToContent button to SubdomainNavBar

ef495b3

revert organize imports

63a9363

add changeset

d8e3704

address PR feedback

a7e0008

useId

96d963d

joseph-lozano force-pushed the joseph-lozano/start-of-content-btn branch from 502d4b6 to 96d963d Compare December 14, 2023 15:46

joseph-lozano had a problem deploying to github-pages December 14, 2023 15:54 — with GitHub Actions Failure

joseph-lozano had a problem deploying to github-pages December 14, 2023 17:02 — with GitHub Actions Failure

give startOfContent div tabIndex -1

0e40741

joseph-lozano commented Dec 14, 2023

View reviewed changes

joseph-lozano had a problem deploying to github-pages December 14, 2023 17:26 — with GitHub Actions Failure

stamat added 2 commits February 26, 2024 14:46

Merge branch 'main' into joseph-lozano/start-of-content-btn

7ea9d42

correctly merge

b6d9b39

stamat temporarily deployed to github-pages February 26, 2024 15:07 — with GitHub Actions Inactive

npm update

2ca4822

stamat temporarily deployed to github-pages February 26, 2024 15:39 — with GitHub Actions Inactive

use reference for main element

ddd5b28

stamat temporarily deployed to github-pages February 28, 2024 20:06 — with GitHub Actions Inactive

stamat marked this pull request as ready for review February 28, 2024 20:17

Rename the two test stories

82eeea1

stamat self-requested a review February 28, 2024 20:59

stamat temporarily deployed to github-pages February 28, 2024 21:05 — with GitHub Actions Inactive

Merge branch 'main' into joseph-lozano/start-of-content-btn

101c07f

stamat temporarily deployed to github-pages March 4, 2024 11:40 — with GitHub Actions Inactive

stamat self-assigned this Mar 4, 2024

npm update

aefedc6

stamat temporarily deployed to github-pages March 4, 2024 23:41 — with GitHub Actions Inactive

stamat added 2 commits March 5, 2024 16:20

ignore role main confict

02c76ed

sorry prettier I forgot a trailing space

c94d940

stamat temporarily deployed to github-pages March 5, 2024 15:35 — with GitHub Actions Inactive

stamat mentioned this pull request Mar 5, 2024

axe:test target individual stories #546

Merged

9 tasks

Merge branch 'main' into joseph-lozano/start-of-content-btn

3e86b7d

stamat temporarily deployed to github-pages March 6, 2024 21:04 — with GitHub Actions Inactive

stamat merged commit 3098b3b into main Mar 8, 2024
16 of 17 checks passed

stamat deleted the joseph-lozano/start-of-content-btn branch March 8, 2024 11:28

primer-css mentioned this pull request Mar 8, 2024

Release tracking #549

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add SkipToContent button to SubdomainNavBar #503

add SkipToContent button to SubdomainNavBar #503

joseph-lozano commented Nov 22, 2023 •

edited

Loading

changeset-bot bot commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

Example:

rezrah Nov 30, 2023

rezrah Nov 30, 2023

rezrah Dec 4, 2023

rezrah Dec 7, 2023

nsolerieu commented Dec 4, 2023

rezrah Dec 7, 2023

rezrah left a comment

joseph-lozano commented Dec 13, 2023

joseph-lozano Dec 14, 2023

rezrah Dec 14, 2023 •

edited

Loading

stamat Feb 28, 2024 •

edited

Loading

stamat commented Mar 5, 2024 •

edited

Loading

		onFocus={() => setStartOfContentButtonFocused(true)}
		onBlur={() => setStartOfContentButtonFocused(false)}

add SkipToContent button to SubdomainNavBar #503

add SkipToContent button to SubdomainNavBar #503

Conversation

joseph-lozano commented Nov 22, 2023 • edited Loading

Summary

List of notable changes:

What should reviewers focus on?

Steps to test:

Supporting resources (related issues, external links, etc):

Contributor checklist:

Reviewer checklist:

Screenshots:

changeset-bot bot commented Nov 22, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 22, 2023 • edited Loading

🟢 No design token changes found

github-actions bot commented Nov 22, 2023 • edited Loading

⚠️ Visual differences found

Example:

rezrah Nov 30, 2023

Choose a reason for hiding this comment

rezrah Nov 30, 2023

Choose a reason for hiding this comment

rezrah Dec 4, 2023

Choose a reason for hiding this comment

rezrah Dec 7, 2023

Choose a reason for hiding this comment

nsolerieu commented Dec 4, 2023

rezrah Dec 7, 2023

Choose a reason for hiding this comment

rezrah left a comment

Choose a reason for hiding this comment

joseph-lozano commented Dec 13, 2023

joseph-lozano Dec 14, 2023

Choose a reason for hiding this comment

rezrah Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

stamat Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

stamat commented Mar 5, 2024 • edited Loading

joseph-lozano commented Nov 22, 2023 •

edited

Loading

changeset-bot bot commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023 •

edited

Loading

rezrah Dec 14, 2023 •

edited

Loading

stamat Feb 28, 2024 •

edited

Loading

stamat commented Mar 5, 2024 •

edited

Loading