Token: Add text alternative to close control #4799

TylerJDev · 2024-07-31T13:28:20Z

Closes https://github.com/github/primer/issues/3582 https://github.com/github/primer/issues/3443

Changelog

New

Add new prop removeButtonLabel

Changed

Moved visually hidden text into token container

Removed

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-07-31T13:28:23Z

⚠️ No Changeset found

Latest commit: 08a9448

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2024-07-31T13:32:14Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	97.05 KB (-0.02% 🔽)
packages/react/dist/browser.umd.js	97.46 KB (-0.01% 🔽)

siddharthkp · 2024-08-13T15:08:55Z

packages/react/src/Token/Token.features.stories.tsx

    >
      <Token as="a" href="/?path=/story/components-token-features--issue-label-token-custom-colors" text="Link" />
      <Token as="button" onClick={action('clicked')} text="Button" />
-      <Token as="span" tabIndex={0} onFocus={action('focused')} text="Focusable Span" />


Is this change intentional? Seeing a lot of visual snapshot tests changed, but not sure if that's part of the PR

Update: Sorry, I understood why we are removing this! Didn't realise it was span with tab-index 😅🤷‍♀️

siddharthkp · 2024-08-13T15:19:06Z

packages/react/src/Token/Token.tsx

    onRemove,
    id,
    leadingVisual: LeadingVisual,
+    removeButtonLabel = 'Remove token',


Just wondering, is this a safe default? Where are Tokens used in dotcom and would it be obvious what "token" is referring to?

Open questions:

Is no default better than confusing default?

Is there a better default like Remove ${props.text}?

Yup that's fair! I only kept it as Remove token since that was the default aria-label on it. We can definitely customize it a bit more, Remove "${props.text}" token.

Is no default better than confusing default?

Yeah, making consumers add the label themselves is sometimes the better option. In this case, since it's already using an aria-label="Remove token", I'm thinking the most backwards compatible way is to make it customizable while keeping the default? 🤔

Where are Tokens used in dotcom and would it be obvious what "token" is referring to?

Usage looks pretty limited ~ 3 instances, which is good! I think by adding ${props.text}, the "token" should be described in a way that users will know what it is.

siddharthkp · 2024-08-13T15:25:52Z

packages/react/src/Token/Token.docs.json

+      "name": "removeButtonLabel",
+      "type": "string",
+      "defaultValue": "'Remove token'",
+      "description": "Label to give to the remove button for assistive technologies, such as screen readers"


I think this API is becoming cumbersome because it feels like it's an after thought

We have 3 props onRemove, hideRemoveButton and removeButtonLabel that all affect one button and clash with each other :(

I'd love to clean this up in a backward compatible way. Do you think we could/should address that as part of this work?

This is a good point! I had a lot more planned for this PR, but unfortunately this is so intertwined with TextInputWithTokens it was hard to add meaningful changes without breaking something in that component 😕

I do think that we'd want to keep removeButtonLabel as there's no current way to change the "Remove token" aria-label attached on the token button now. I was planning on coming back to Token in https://github.com/github/primer/issues/3468, as this issue has the potential to require a lot of changes to both components.

We could remove removeButtonLabel and handle it later since that isn't part of any of the issues in https://github.com/github/primer/issues/3571, but an accessibility enhancement, or we could ship these changes and work towards making this component a bit cleaner and scope that into https://github.com/github/primer/issues/3468. Curious what you think though!

We could remove removeButtonLabel and handle it later ... and work towards making this component a bit cleaner and scope that into https://github.com/github/primer/issues/3468.

Sounds like a good plan! That will give us a good way to look at the API together

siddharthkp

Left a couple questions

TylerJDev · 2024-08-20T14:12:52Z

@siddharthkp, with the idea of us rescoping some of the work needed for Token in https://github.com/github/primer/issues/3468, I'm wondering if this PR is good to be shipped? Since the two are intertwined, I was going to work towards adding more changes to Token in that issue, as the additions in this PR wouldn't need to be changed.

siddharthkp

request 1 change for removeButtonLabel, everything else looks great!

siddharthkp · 2024-09-11T14:24:02Z

packages/react/src/Token/Token.docs.json

+      "name": "removeButtonLabel",
+      "type": "string",
+      "defaultValue": "'Remove token'",
+      "description": "Label to give to the remove button for assistive technologies, such as screen readers"


We could remove removeButtonLabel and handle it later ... and work towards making this component a bit cleaner and scope that into https://github.com/github/primer/issues/3468.

Sounds like a good plan! That will give us a good way to look at the API together

TylerJDev · 2024-09-11T17:15:02Z

@siddharthkp, removed all instances of the prop! Should only consist of the storybook changes now

siddharthkp · 2024-09-12T09:05:01Z

packages/react/src/Token/Token.tsx

+      <TokenTextContainer {...(hasMultipleActionTargets ? interactiveTokenProps : {})}>
+        {text}
+        {onRemove && <VisuallyHidden> (press backspace or delete to remove)</VisuallyHidden>}
+      </TokenTextContainer>


Just confirming this change: What does bringing onRemove inside the TokenTextContainer give us?

This one was a suggestion in this issue https://github.com/github/primer/issues/3582.

We're essentially adding the "(press backspace or delete to remove)" as part of the accessible name of the control instead of it being adjacent. This means that if a user using a screen reader tabs into a button token, their screen reader should announce {text} (press backspace or delete to remove). Outside of this PR, it can be easy to skip that text if you're just tabbing through interactive tokens, as you have to manually navigate by text to reach the message with a screen reader.

Now that I think about it a bit more, we could also use aria-describedby if we don't want to make this part of the accessible name, but also want to ensure that it won't be as easy to skip as it is currently 🤔

Ah, understood.

Let's run integration once just to make sure there are no jest tests that expect just the text to be present in the token

siddharthkp

Looks great!

Integration tests suggested, approving in advance

primer-integration · 2024-09-12T12:41:15Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/341786

TylerJDev · 2024-09-12T15:12:05Z

Tested in gh/gh, and integration tests look good! https://github.com/github/github/pull/341786

* Add new prop to `Token` * Remove `console.log` * Move text into control * Remove inaccessible examples * Add prop to docs * Add changeset * Update snapshots * Update text on prop * test(vrt): update snapshots * remove `removeButtonLabel` * Update snapshot * test(vrt): update snapshots --------- Co-authored-by: TylerJDev <TylerJDev@users.noreply.github.com>

TylerJDev added 2 commits July 30, 2024 14:52

Add new prop to Token

f23b955

Remove console.log

7b0cdd3

TylerJDev added the accessibility label Jul 31, 2024

TylerJDev self-assigned this Jul 31, 2024

TylerJDev temporarily deployed to github-pages July 31, 2024 13:32 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4799 July 31, 2024 13:33 Inactive

TylerJDev added 3 commits July 31, 2024 09:35

Move text into control

e072819

Remove inaccessible examples

e69b804

Add prop to docs

47508a8

TylerJDev temporarily deployed to github-pages July 31, 2024 13:45 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4799 July 31, 2024 13:45 Inactive

Add changeset

10c7e6a

TylerJDev temporarily deployed to github-pages August 1, 2024 20:34 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4799 August 1, 2024 20:34 Inactive

Update snapshots

ae4f5a9

TylerJDev temporarily deployed to github-pages August 1, 2024 20:43 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4799 August 1, 2024 20:44 Inactive

Update text on prop

629d0f7

TylerJDev temporarily deployed to github-pages August 5, 2024 14:13 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4799 August 5, 2024 14:14 Inactive

TylerJDev added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Aug 5, 2024

test(vrt): update snapshots

72da3d5

github-actions bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Aug 5, 2024

TylerJDev marked this pull request as ready for review August 5, 2024 14:29

TylerJDev requested review from a team as code owners August 5, 2024 14:29

TylerJDev requested a review from langermank August 5, 2024 14:29

primer bot temporarily deployed to github-pages August 5, 2024 14:30 Inactive

github-actions bot temporarily deployed to storybook-preview-4799 August 5, 2024 14:31 Inactive

TylerJDev requested a review from siddharthkp August 5, 2024 15:20

siddharthkp reviewed Aug 13, 2024

View reviewed changes

TylerJDev requested a review from siddharthkp September 10, 2024 17:40

siddharthkp requested changes Sep 11, 2024

View reviewed changes

remove removeButtonLabel

0437083

TylerJDev added the skip changeset This change does not need a changelog label Sep 11, 2024

TylerJDev added 2 commits September 11, 2024 12:46

Update snapshot

9075106

Merge branch 'main' into tylerjdev/token-a11y-fixes

59e367e

TylerJDev added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Sep 11, 2024

TylerJDev temporarily deployed to github-pages September 11, 2024 16:49 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4799 September 11, 2024 16:49 Inactive

test(vrt): update snapshots

08a9448

github-actions bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Sep 11, 2024

primer bot deployed to github-pages September 11, 2024 16:57 View deployment

github-actions bot deployed to storybook-preview-4799 September 11, 2024 16:58 View deployment

siddharthkp reviewed Sep 12, 2024

View reviewed changes

siddharthkp approved these changes Sep 12, 2024

View reviewed changes

TylerJDev added this pull request to the merge queue Sep 12, 2024

Merged via the queue into main with commit f1196a8 Sep 12, 2024

TylerJDev deleted the tylerjdev/token-a11y-fixes branch September 12, 2024 15:26

Token: Add text alternative to close control #4799

Token: Add text alternative to close control #4799

Uh oh!

Conversation

TylerJDev commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

Uh oh!

changeset-bot bot commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions bot commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

siddharthkp Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkp left a comment

Choose a reason for hiding this comment

Uh oh!

TylerJDev commented Aug 20, 2024

Uh oh!

siddharthkp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerJDev commented Sep 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkp left a comment

Choose a reason for hiding this comment

Uh oh!

primer-integration bot commented Sep 12, 2024

Uh oh!

TylerJDev commented Sep 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TylerJDev commented Jul 31, 2024 •

edited

Loading

changeset-bot bot commented Jul 31, 2024 •

edited

Loading

github-actions bot commented Jul 31, 2024 •

edited

Loading

siddharthkp Aug 13, 2024 •

edited

Loading