Allows asterisk in `FormControl.Label` to be adjustable by TylerJDev · Pull Request #4543 · primer/react

TylerJDev · 2024-04-29T18:56:10Z

Closes: https://github.com/github/primer/issues/3263
https://github.com/github/accessibility-audits/issues/5724

Provides more flexibility to our required indicator (currently an asterisk). This PR adds the ability to change the text of the indicator, as well as hide it from AT via the new requiredIndicator prop.

Changelog

New

Adds two new props in FormControl.Label:

requiredText - Allows you to change the required indicator from '*', to the value provided. Defaults to '*'.
requiredIndicator - Allows you to toggle between the indicator being present or not in the accessibility tree. Defaults to 'true'.

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-04-29T18:56:14Z

🦋 Changeset detected

Latest commit: be9b4a6

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	Patch

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 · 2024-04-29T18:59:40Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	89.34 KB (-0.02% 🔽)
packages/react/dist/browser.umd.js	89.68 KB (+0.02% 🔺)

TylerJDev · 2024-04-30T14:20:23Z

+ requiredText?: string
+ requiredIndicator?: boolean


I'm wondering if this might be better handled on <FormControl> itself, as to apply the required indicator settings to all labels that exist within. The main con here is that there could be cases where you'd only want to adjust one of the indicators, not all.

It makes sense to me to add this to the FormControl.Label 🤔 Because that's where the indicator is. I could see users wanting to be able to set a default requiredText for an entire form but it looks like there's no Form element so that's not really possible. I guess in that instance an engineer would probably create a custom component which inherits from the FormControl.Label.

Yeah I agree! I think if there's truly value here we could come back to it, but for now I think handling it on FormControl.Label is good enough.

TylerJDev · 2024-04-30T14:30:47Z

 <Box display="flex" as="span">
 <Box mr={1}>{children}</Box>
- <span>*</span>
+{requiredText && <span aria-hidden={!requiredIndicator ? true : undefined}>{requiredText}</span>}


Right now requiredIndicator only hides the indicator to the accessibility tree (i.e. aria-hidden="true"). This is because the indicator is usually important in discerning which form fields are required visually, whereas it being present in the accessibility tree is not.
You can still technically hide the indicator by using an empty string in requiredText (e.g. requiredText="").

Do we want to allow an empty requiredText when the element is required 🤔 I think I would be in favour here of that not being possible. I don't feel super strongly about this but it could lead to audit issues in future if someone hides the requiredText but sets a field to be programatically required.

That's a good point, I'm not entirely sure if there are instances today where we'd want to opt out of a visual indicator for required fields. The only instance that I could think of, is if someone were to opt for marking optional fields visually rather than required fields (e.g. (optional)). I don't think we have a case like that in GH today, so I'm okay with defaulting to '*' if requiredText is an empty string.

TylerJDev · 2024-04-30T14:48:55Z

This is one of many ways to handle how we do required indicators in FormControl.Label. Would love any other opinions on how to make this easier for consumers 👀

More context: https://github.com/github/primer/issues/3263

owenniblock

Looks good to me, one comment on the edge case of setting requiredText to be empty. Also a nitpick on the docs wording 🙂

owenniblock · 2024-05-07T10:26:32Z

+ requiredText?: string
+ requiredIndicator?: boolean


It makes sense to me to add this to the FormControl.Label 🤔 Because that's where the indicator is. I could see users wanting to be able to set a default requiredText for an entire form but it looks like there's no Form element so that's not really possible. I guess in that instance an engineer would probably create a custom component which inherits from the FormControl.Label.

owenniblock · 2024-05-07T10:32:00Z

 <Box display="flex" as="span">
 <Box mr={1}>{children}</Box>
- <span>*</span>
+{requiredText && <span aria-hidden={!requiredIndicator ? true : undefined}>{requiredText}</span>}


Do we want to allow an empty requiredText when the element is required 🤔 I think I would be in favour here of that not being possible. I don't feel super strongly about this but it could lead to audit issues in future if someone hides the requiredText but sets a field to be programatically required.

Bumps [ejs](https://github.com/mde/ejs) from 3.1.9 to 3.1.10. - [Release notes](https://github.com/mde/ejs/releases) - [Commits](mde/ejs@v3.1.9...v3.1.10) --- updated-dependencies: - dependency-name: ejs dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* add style for branchName as span adn add v8 tokens * added changeset * Update thin-ligers-turn.md * test(vrt): update snapshots * use not a instead of matching for span --------- Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: lukasoppermann <lukasoppermann@users.noreply.github.com>

* refactor(Banner): update region to use a dedicated aria-label * chore: add changeset, update config to exclude codesandbox * feat: add aria-label to Banner * Update packages/react/src/Banner/Banner.test.tsx Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> * Update packages/react/src/Banner/Banner.test.tsx Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> * test: update test label with aria-label --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>

Co-authored-by: Owen Niblock <owenniblock@github.com>

* Add new props to `FormControl.Label` * Add conditional * Add changeset * Update docs json * Add more examples * chore(deps-dev): bump ejs from 3.1.9 to 3.1.10 (#4549) Bumps [ejs](https://github.com/mde/ejs) from 3.1.9 to 3.1.10. - [Release notes](https://github.com/mde/ejs/releases) - [Commits](mde/ejs@v3.1.9...v3.1.10) --- updated-dependencies: - dependency-name: ejs dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * BranchName: Add style for span and add v8 tokens (#4556) * add style for branchName as span adn add v8 tokens * added changeset * Update thin-ligers-turn.md * test(vrt): update snapshots * use not a instead of matching for span --------- Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: lukasoppermann <lukasoppermann@users.noreply.github.com> * refactor(Banner): update region to use a dedicated aria-label (#4539) * refactor(Banner): update region to use a dedicated aria-label * chore: add changeset, update config to exclude codesandbox * feat: add aria-label to Banner * Update packages/react/src/Banner/Banner.test.tsx Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> * Update packages/react/src/Banner/Banner.test.tsx Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> * test: update test label with aria-label --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> * chore(deps-dev): bump cross-env from 7.0.2 to 7.0.3 (#4561) Bumps [cross-env](https://github.com/kentcdodds/cross-env) from 7.0.2 to 7.0.3. - [Release notes](https://github.com/kentcdodds/cross-env/releases) - [Changelog](https://github.com/kentcdodds/cross-env/blob/master/CHANGELOG.md) - [Commits](kentcdodds/cross-env@v7.0.2...v7.0.3) --- updated-dependencies: - dependency-name: cross-env dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump @babel/plugin-transform-modules-commonjs (#4562) Bumps [@babel/plugin-transform-modules-commonjs](https://github.com/babel/babel/tree/HEAD/packages/babel-plugin-transform-modules-commonjs) from 7.23.3 to 7.24.1. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.24.1/packages/babel-plugin-transform-modules-commonjs) --- updated-dependencies: - dependency-name: "@babel/plugin-transform-modules-commonjs" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump jest-fail-on-console from 3.1.1 to 3.2.0 (#4563) Bumps [jest-fail-on-console](https://github.com/ValentinH/jest-fail-on-console) from 3.1.1 to 3.2.0. - [Release notes](https://github.com/ValentinH/jest-fail-on-console/releases) - [Commits](ValentinH/jest-fail-on-console@v3.1.1...v3.2.0) --- updated-dependencies: - dependency-name: jest-fail-on-console dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat(FeatureFlags): broaden feature flag type to accept undefined (#4554) * feat(FeatureFlags): loosen feature flag type to accept undefined * chore: add changeset * Update .changeset/grumpy-coats-worry.md --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> * prevent form submit (#4551) * deprecate title prop on ActionList.Group component on docs (#4544) * chore: add hydro analytics to storybook (#4558) * chore: add hydro analytics to storybook * chore: use previewHead over managerHead * Update build-docs --------- Co-authored-by: Josh Black <joshblack@users.noreply.github.com> * Revert "Revert "Add support for nested submenus to `ActionMenu`"" (#4486) * Revert "Revert "Add support for nested submenus to `ActionMenu` (#4386)" (#4472)" This reverts commit 82072eb. * just want a change to trigger rebuild --------- Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: Pavithra Kodmad <pksjce@github.com> * chore(deps): update typescript to 5.4.5 (#4568) Co-authored-by: Josh Black <joshblack@users.noreply.github.com> * Use dynamic height and width for dialogs (#4567) * Use dynamic height and width for dialogs * Update tall-forks-bathe.md --------- Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> * Make asterisk default, update story scenarios * Update packages/react/src/FormControl/FormControl.docs.json Co-authored-by: Owen Niblock <owenniblock@github.com> * Update packages/react/src/internal/components/InputLabel.tsx Co-authored-by: Owen Niblock <owenniblock@github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lukas Oppermann <lukasoppermann@github.com> Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: lukasoppermann <lukasoppermann@users.noreply.github.com> Co-authored-by: Josh Black <joshblack@github.com> Co-authored-by: Josh Black <joshblack@users.noreply.github.com> Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com> Co-authored-by: Armağan <broccolinisoup@github.com> Co-authored-by: Ian Sanders <iansan5653@github.com> Co-authored-by: Pavithra Kodmad <pksjce@github.com> Co-authored-by: Dusty Greif <dgreif@users.noreply.github.com> Co-authored-by: Owen Niblock <owenniblock@github.com>

Add new props to FormControl.Label
ad64388

TylerJDev temporarily deployed to github-pages April 29, 2024 18:59 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 April 29, 2024 19:00 Inactive

Add conditional
3ab4b1e

TylerJDev temporarily deployed to github-pages April 29, 2024 21:03 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 April 29, 2024 21:03 Inactive

TylerJDev commented Apr 30, 2024
View reviewed changes

Add changeset
413ac1e

TylerJDev temporarily deployed to github-pages April 30, 2024 14:41 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 April 30, 2024 14:41 Inactive

TylerJDev marked this pull request as ready for review April 30, 2024 14:49

TylerJDev requested a review from a team as a code owner April 30, 2024 14:49

TylerJDev requested a review from owenniblock April 30, 2024 14:49

Update docs json
add9be3

TylerJDev temporarily deployed to github-pages May 1, 2024 13:27 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 May 1, 2024 13:28 Inactive

Add more examples
fc84578

TylerJDev temporarily deployed to github-pages May 1, 2024 13:40 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 May 1, 2024 13:40 Inactive

Merge branch 'main' into form-control-required-indicator
4d31493

TylerJDev temporarily deployed to github-pages May 2, 2024 13:59 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 May 2, 2024 14:00 Inactive

owenniblock reviewed May 7, 2024
View reviewed changes

dependabotBotand others added 3 commits May 7, 2024 14:31

TylerJDev temporarily deployed to github-pages May 7, 2024 18:39 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 May 7, 2024 18:39 Inactive

TylerJDev marked this pull request as ready for review May 7, 2024 18:43

TylerJDev requested review from a team and owenniblock and removed request for a team and maximedegreve May 7, 2024 18:43

Merge branch 'main' into form-control-required-indicator
0d0b531

TylerJDev temporarily deployed to github-pages May 9, 2024 15:10 — with GitHub Actions Inactive

github-actionsBottemporarily deployed to storybook-preview-4543 May 9, 2024 15:11 Inactive

owenniblock approved these changes May 23, 2024
View reviewed changes

Comment threadpackages/react/src/internal/components/InputLabel.tsx Outdated

TylerJDevand others added 2 commits May 23, 2024 12:23

Update packages/react/src/FormControl/FormControl.docs.json
afe895d
Co-authored-by: Owen Niblock <owenniblock@github.com>

Update packages/react/src/internal/components/InputLabel.tsx
be9b4a6
Co-authored-by: Owen Niblock <owenniblock@github.com>

TylerJDev deployed to github-pages May 23, 2024 16:30 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-4543 May 23, 2024 16:30 Inactive

TylerJDev added this pull request to the merge queue May 23, 2024

Merged via the queue into main with commit e47445dMay 23, 2024

TylerJDev deleted the form-control-required-indicator branch May 23, 2024 16:51

primerBot mentioned this pull request May 23, 2024
Release tracking (36.19.1) #4616
Merged

JonathanXDR mentioned this pull request Jun 18, 2024
[Snyk] Upgrade @primer/react from 36.18.0 to 36.19.1 JonathanXDR/Atom#29
Closed

OATEF4313 mentioned this pull request Jun 20, 2024
[Snyk] Upgrade @primer/react from 36.19.0 to 36.19.1 OATEF4313/docs#3
Open

This was referenced Jun 20, 2024
[Snyk] Upgrade @primer/react from 36.18.0 to 36.19.1 JonathanXDR/Atom#30
Closed
[Snyk] Upgrade @primer/react from 36.18.0 to 36.20.0 JonathanXDR/Atom#31
Closed

StemmlerSisters mentioned this pull request Jul 16, 2024
[Snyk] Upgrade @primer/react from 36.1.0 to 36.23.0 StemmlerSisters/documentation#5
Open

snyk-ioBot mentioned this pull request Jul 17, 2024
[Snyk] Upgrade @primer/react from 36.11.0 to 36.23.0 Hawthorne001/documentation.npm#3
Open

julitabarelkowska mentioned this pull request Jul 17, 2024
[Snyk] Upgrade @primer/react from 36.11.0 to 36.23.0 julitabarelkowska/documentation#7
Open

Canadatechmanufacturing mentioned this pull request Jul 17, 2024
[Snyk] Upgrade @primer/react from 36.7.0 to 36.23.0 Canadatechmanufacturing/documentation#3
Open

snyk-ioBot mentioned this pull request Aug 13, 2024
[Snyk] Upgrade @primer/react from 36.19.0 to 36.26.0 Playgirlkaybraz11/docs#2
Open

Navigation Menu

Uh oh!

Allows asterisk in `FormControl.Label` to be adjustable - #4543

Allows asterisk in `FormControl.Label` to be adjustable#4543
TylerJDev merged 24 commits into
mainfrom
form-control-required-indicator

TylerJDev commented Apr 29, 2024•
edited
Loading

Uh oh!

changeset-botBot commented Apr 29, 2024•
edited
Loading

Uh oh!

github-actionsBot commented Apr 29, 2024•
edited
Loading

Uh oh!

TylerJDevApr 30, 2024

Uh oh!

owenniblockMay 7, 2024

Uh oh!

TylerJDevMay 7, 2024

Uh oh!

TylerJDevApr 30, 2024

Uh oh!

owenniblockMay 7, 2024

Uh oh!

TylerJDevMay 7, 2024

Uh oh!

TylerJDev commented Apr 30, 2024

Uh oh!

owenniblock left a comment

Uh oh!

Uh oh!

owenniblockMay 7, 2024

Uh oh!

owenniblockMay 7, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

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

Uh oh!

Changelog

New

Rollout strategy

Testing & Reviewing

Merge checklist

Uh oh!

changeset-botBot commented Apr 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actionsBot commented Apr 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

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!

TylerJDev commented Apr 30, 2024

Uh oh!

owenniblock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

TylerJDev commented Apr 29, 2024•
edited
Loading

changeset-botBot commented Apr 29, 2024•
edited
Loading

github-actionsBot commented Apr 29, 2024•
edited
Loading