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

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. 
        
            */ 
        
           export function useFormControlForwardedProps<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. 
        
            */ 
        
           export function useFormControlForwardedProps(): FormControlForwardedProps 
        
           export function useFormControlForwardedProps(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

packages/react/src/ActionMenu/ActionMenu.tsx

 }: ActionMenuProps) => {
  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

const inputProps = useFormControlForwardedProps(externalInputProps)

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

packages/react/src/ActionMenu/ActionMenu.tsx

@@ -69,7 +72,7 @@ const Menu: React.FC<React.PropsWithChildren<ActionMenuProps>> = ({
        renderAnchor = anchorProps => {
          // 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

packages/react/src/Token/__tests__/__snapshots__/Token.test.tsx.snap

@@ -22,20 +22,6 @@ exports[`Token components AvatarToken renders all sizes 1`] = `
  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

packages/react/src/__tests__/ActionMenu.test.tsx

+    </FormControl>
+  )
+}
+


I tried adding a test case for TooltipV2

function ExampleWithTooltipV2WithForm(): JSX.Element { return ( <FormControl> <FormControl.Label>Action Menu Label</FormControl.Label> <ThemeProvider theme={theme}> <SSRProvider> <BaseStyles> <ActionMenu> <TooltipV2 text="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 () => { const component = HTMLRender(<ExampleWithTooltipV2WithForm />) const button = component.getByLabelText('Action Menu Label') expect(button).toBeVisible() const buttonByRole = 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

function ExampleWithTooltipV2WithForm(): JSX.Element { return ( <FormControl> <FormControl.Label>Action Menu Label</FormControl.Label> <ThemeProvider theme={theme}> <SSRProvider> <BaseStyles> <TooltipV2 text="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-actions bot temporarily 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 change Mar 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-actions bot temporarily 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-actions bot temporarily 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 Component Mar 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-actions bot temporarily 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-actions bot temporarily deployed to storybook-preview-4372 March 12, 2024 21:33 Inactive

primer deleted a comment from github-actions bot Mar 12, 2024

updated failing snapshot test

14f60fe

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

github-actions bot deployed to storybook-preview-4372 March 13, 2024 14:26 View deployment

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JeffreyKozik commented Mar 8, 2024 •

edited

Loading

changeset-bot bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

siddharthkp commented Mar 13, 2024 •

edited

Loading

JeffreyKozik Mar 13, 2024

JeffreyKozik Mar 13, 2024

JeffreyKozik Mar 13, 2024

JeffreyKozik Mar 13, 2024 •

edited

Loading

JeffreyKozik commented Mar 13, 2024 •

edited

Loading

	/**
	* 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.
	*/
	export function useFormControlForwardedProps<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.
	*/
	export function useFormControlForwardedProps(): FormControlForwardedProps
	export function useFormControlForwardedProps(externalProps: FormControlForwardedProps = {}) {

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

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

Conversation

JeffreyKozik commented Mar 8, 2024 • edited Loading

Changelog

New

Changed

Removed

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Mar 8, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Mar 8, 2024 • edited Loading

size-limit report 📦

siddharthkp commented Mar 13, 2024 • edited Loading

JeffreyKozik Mar 13, 2024

Choose a reason for hiding this comment

JeffreyKozik Mar 13, 2024

Choose a reason for hiding this comment

JeffreyKozik Mar 13, 2024

Choose a reason for hiding this comment

JeffreyKozik Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

JeffreyKozik commented Mar 13, 2024 • edited Loading

JeffreyKozik commented Mar 8, 2024 •

edited

Loading

changeset-bot bot commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

siddharthkp commented Mar 13, 2024 •

edited

Loading

JeffreyKozik Mar 13, 2024 •

edited

Loading

JeffreyKozik commented Mar 13, 2024 •

edited

Loading