Adds FormControl's "Auto-Wiring" to ActionMenu Component by JeffreyKozik · Pull Request #4372 · primer/react

JeffreyKozik · 2024-03-08T21:57:45Z

Followup to previous discussions:

Changelog

New

Changed

Made use of the the new useFormControlForwardedProps hook (introduced by #3632) to automatically wire the ActionMenu component to the accessibility and validation provided by the FormControl component it's nested within. If ActionMenu isn't nested within FormControl the hook has no effect.

react/packages/react/src/FormControl/_FormControlContext.tsx

Lines 24 to 39 in f7dcae9

/**
 * Make any component compatible with `FormControl`'s automatic wiring up of accessibility attributes & validation by
 * reading the props from this hook and merging them with the passed-in props. If used outside of `FormControl`, this
 * hook has no effect.
 *
 * @param externalProps The external props passed to this component. If provided, these props will be merged with the
 * `FormControl` props, with external props taking priority.
 */
exportfunctionuseFormControlForwardedProps<P>(externalProps: P): P&FormControlForwardedProps
/**
 * Make any component compatible with `FormControl`'s automatic wiring up of accessibility attributes & validation by
 * reading the props from this hook and handling them / assigning them to the underlying form control. If used outside
 * of `FormControl`, this hook has no effect.
 */
exportfunctionuseFormControlForwardedProps(): FormControlForwardedProps
exportfunctionuseFormControlForwardedProps(externalProps: FormControlForwardedProps={}){

Within a form the accessible name is now the text within the corresponding FormControl.Label tag

Outside of a form the accessible name is still the text within the ActionMenu.Button itself

I also updated Jest snapshots for the Token component (all it did was rearrange class names). I'm unsure why it did this since it seems unrelated to the changes I made, but I'm not too concerned about it since we seem to be moving away from snapshot testing: https://github.com/primer/react/blob/cb54f4298700d369e616aaefb2e7df4830231736/contributor-docs/testing.md#snapshots.

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-03-08T21:57:49Z

🦋 Changeset detected

Latest commit: 14f60fe

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 · 2024-03-08T22:01:10Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	87.51 KB (-0.08% 🔽)
packages/react/dist/browser.umd.js	87.93 KB (-0.03% 🔽)

…_formcontrollabel

siddharthkp · 2024-03-13T14:09:26Z

Thanks for creating a PR!

Leaving rewatch link for myself to catch up: https://github.rewatch.com/video/2xqmd8rdinwuuedx-accessibility-team-eng-office-hours-march-11-2024?t=60.69

JeffreyKozik · 2024-03-13T15:17:39Z

 const [combinedOpenState, setCombinedOpenState] = useProvidedStateOrCreate(open, onOpenChange, false)
 const onOpen = React.useCallback(() => setCombinedOpenState(true), [setCombinedOpenState])
 const onClose = React.useCallback(() => setCombinedOpenState(false), [setCombinedOpenState])
+ const inputProps = useFormControlForwardedProps(externalInputProps)


Using the hook recently created by this PR: #3632. This hook is also being used in the InlineAutocomplete draft component:
react/packages/react/src/drafts/InlineAutocomplete/InlineAutocomplete.tsx
Line 118 in cb54f42
constinputProps=useFormControlForwardedProps(externalInputProps)

JeffreyKozik · 2024-03-13T15:18:09Z

 // We need to attach the anchor props to the tooltip trigger (ActionMenu.Button's grandchild) not the tooltip itself.
 const triggerButton = React.cloneElement(anchorChildren, {...anchorProps})
- return React.cloneElement(child, {children: triggerButton, ref: anchorRef})
+ return React.cloneElement(child, {children: triggerButton, ref: anchorRef, ...inputProps})


Followed the pattern suggested in this slack thread: https://github.slack.com/archives/CSGAVNZ19/p1709843761424779?thread_ts=1709836721.563139&cid=CSGAVNZ19

JeffreyKozik · 2024-03-13T15:19:53Z

 border-width: 0;
 }

-.c3 {


I updated snapshots as detailed in the contributor docs: https://github.com/primer/react/blob/cb54f4298700d369e616aaefb2e7df4830231736/contributor-docs/testing.md#snapshots
Not sure why these classes had to be rearranged as it's not clear to me how Token relates to my ActionMenu changes at all

JeffreyKozik · 2024-03-13T15:27:31Z

+ </FormControl>
+ )
+}
+


I tried adding a test case for TooltipV2
functionExampleWithTooltipV2WithForm(): JSX.Element{return(<FormControl><FormControl.Label>Action Menu Label</FormControl.Label><ThemeProvidertheme={theme}><SSRProvider><BaseStyles><ActionMenu><TooltipV2text="Additional context about the menu button"direction="s"><ActionMenu.Button>Toggle Menu</ActionMenu.Button></TooltipV2><ActionMenu.Overlay><ActionList><ActionList.Item>New file</ActionList.Item></ActionList></ActionMenu.Overlay></ActionMenu></BaseStyles></SSRProvider></ThemeProvider></FormControl>)}
it('MenuButton wrapped by TooltipV2 within FormControl should be labelled by FormControl.Label',async()=>{constcomponent=HTMLRender(<ExampleWithTooltipV2WithForm/>)constbutton=component.getByLabelText('Action Menu Label')expect(button).toBeVisible()constbuttonByRole=component.getByRole('button')expect(button.id).toBe(buttonByRole.id)})
But I got this error because the FormControl.Label ended up labelling the TooltipV2 instead of the ActionMenu.Button (presumably since the tooltip is wrapping the button).
If I try to force the TooltipV2 to act similarly to Tooltip (v1) and wrap the entire ActionMenu
functionExampleWithTooltipV2WithForm(): JSX.Element{return(<FormControl><FormControl.Label>Action Menu Label</FormControl.Label><ThemeProvidertheme={theme}><SSRProvider><BaseStyles><TooltipV2text="Additional context about the menu button"direction="s"><ActionMenu><ActionMenu.Button>Toggle Menu</ActionMenu.Button><ActionMenu.Overlay><ActionList><ActionList.Item>New file</ActionList.Item></ActionList></ActionMenu.Overlay></ActionMenu></TooltipV2></BaseStyles></SSRProvider></ThemeProvider></FormControl>)}
I get this error since that's not how TooltipV2 is supposed to be used.
Either way I'm not concerned about not having a test including TooltipV2 since I can't imagine a valid use case for having an ActionMenu within a FormControl being labelled by a FormControl.Label and then also adding a tooltip on top. Furthermore our Primer a11y guidelines advise against using tooltips in most circumstances: https://primer.style/guides/accessibility/tooltip-alternatives#why.

JeffreyKozik · 2024-03-13T16:01:17Z

Instead of auto-wiring the ActionMenu component, I wrote #4389 to auto-wire the SelectPanel v2 component since it's more fitting to be used in forms: https://github.slack.com/archives/C01L618AEP9/p1710345435224569?thread_ts=1710344884.849579&cid=C01L618AEP9

adds automatic wiring of a11y attributes to ActionMenu.Button
6ffb7f9

JeffreyKozik requested a review from a team as a code owner March 8, 2024 21:57

JeffreyKozik requested a review from siddharthkp March 8, 2024 21:57

JeffreyKozik marked this pull request as draft March 8, 2024 21:58

JeffreyKozik deployed to github-pages March 8, 2024 22:00 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-4372 March 8, 2024 22:01 Inactive

JeffreyKozik self-assigned this Mar 11, 2024

JeffreyKozik changed the title ~~adds automatic wiring of a11y attributes to ActionMenu.Button~~adds automatic wiring of a11y attributes to ActionMenu.Button primer changeMar 11, 2024

adjusted where the hook is being used
123c69b

JeffreyKozik deployed to github-pages March 11, 2024 18:02 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-4372 March 11, 2024 18:03 Inactive

added tests, added stories, fixed logic
bb023a0

JeffreyKozik deployed to github-pages March 11, 2024 21:25 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-4372 March 11, 2024 21:26 Inactive

JeffreyKozik changed the title ~~adds automatic wiring of a11y attributes to ActionMenu.Button primer change~~Adds FormControl's "Auto-Wiring" to ActionMenu ComponentMar 12, 2024

JeffreyKozik added the skip changeset This change does not need a changelog label Mar 12, 2024

Merge branch 'main' into jeffreykozik/autolinking_actionmenubutton_to…
6dba24c
…_formcontrollabel

JeffreyKozik deployed to github-pages March 12, 2024 18:30 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-4372 March 12, 2024 18:30 Inactive

added changeset
cda649a

JeffreyKozik removed the skip changeset This change does not need a changelog label Mar 12, 2024

JeffreyKozik deployed to github-pages March 12, 2024 21:33 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-4372 March 12, 2024 21:33 Inactive

primer deleted a comment from github-actionsBotMar 12, 2024

updated failing snapshot test
14f60fe

JeffreyKozik deployed to github-pages March 13, 2024 14:25 — with GitHub Actions View deployment

github-actionsBottemporarily deployed to storybook-preview-4372 March 13, 2024 14:26 Inactive

JeffreyKozik commented Mar 13, 2024
View reviewed changes

JeffreyKozik closed this Mar 13, 2024

JeffreyKozik mentioned this pull request Mar 13, 2024
Adds FormControl's "Auto-Wiring" to SelectPanel v2 Component #4389
Merged
13 tasks

jonrohan deleted the jeffreykozik/autolinking_actionmenubutton_to_formcontrollabel branch May 9, 2025 18:36

Navigation Menu

Uh oh!

Adds FormControl's "Auto-Wiring" to ActionMenu Component - #4372

Adds FormControl's "Auto-Wiring" to ActionMenu Component#4372
JeffreyKozik wants to merge 6 commits into
mainfrom
jeffreykozik/autolinking_actionmenubutton_to_formcontrollabel

JeffreyKozik commented Mar 8, 2024•
edited
Loading

Uh oh!

changeset-botBot commented Mar 8, 2024•
edited
Loading

Uh oh!

github-actionsBot commented Mar 8, 2024•
edited
Loading

Uh oh!

siddharthkp commented Mar 13, 2024•
edited
Loading

Uh oh!

JeffreyKozikMar 13, 2024

Uh oh!

JeffreyKozikMar 13, 2024

Uh oh!

JeffreyKozikMar 13, 2024

Uh oh!

JeffreyKozikMar 13, 2024•
edited
Loading

Uh oh!

JeffreyKozik commented Mar 13, 2024•
edited
Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	/**
	* Make any component compatible with `FormControl`'s automatic wiring up of accessibility attributes & validation by
	* reading the props from this hook and merging them with the passed-in props. If used outside of `FormControl`, this
	* hook has no effect.
	*
	* @param externalProps The external props passed to this component. If provided, these props will be merged with the
	* `FormControl` props, with external props taking priority.
	*/
	exportfunctionuseFormControlForwardedProps<P>(externalProps: P): P&FormControlForwardedProps
	/**
	* Make any component compatible with `FormControl`'s automatic wiring up of accessibility attributes & validation by
	* reading the props from this hook and handling them / assigning them to the underlying form control. If used outside
	* of `FormControl`, this hook has no effect.
	*/
	exportfunctionuseFormControlForwardedProps(): FormControlForwardedProps
	exportfunctionuseFormControlForwardedProps(externalProps: FormControlForwardedProps={}){

Uh oh!

Conversation

JeffreyKozik commented Mar 8, 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-botBot commented Mar 8, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

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

Uh oh!

size-limit report 📦

Uh oh!

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

Uh oh!

Uh oh!

JeffreyKozikMar 13, 2024

Choose a reason for hiding this comment

Uh oh!

JeffreyKozikMar 13, 2024

Choose a reason for hiding this comment

Uh oh!

JeffreyKozikMar 13, 2024

Choose a reason for hiding this comment

Uh oh!

JeffreyKozikMar 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!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JeffreyKozik commented Mar 8, 2024•
edited
Loading

changeset-botBot commented Mar 8, 2024•
edited
Loading

github-actionsBot commented Mar 8, 2024•
edited
Loading

siddharthkp commented Mar 13, 2024•
edited
Loading

JeffreyKozikMar 13, 2024•
edited
Loading

JeffreyKozik commented Mar 13, 2024•
edited
Loading