-
Notifications
You must be signed in to change notification settings - Fork 536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds FormControl's "Auto-Wiring" to SelectPanel v2 Component #4389
Changes from 14 commits
0f4cec5
06b048a
5e46bce
d1df0dd
3d69e09
969c97c
4d7b5d7
616ad2b
d26d79a
d6b08a2
6ea27c7
006a61b
e8aa873
672aaab
c3e1594
71ea3b5
a3db265
e6d93e1
38d9748
856e957
eacfa7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@primer/react': minor | ||
--- | ||
|
||
experimental/SelectPanel + FormControl: Automatically wires SelectPanel v2 to the accessibility and validation provided by the FormControl component it's nested within |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,8 +1,21 @@ | ||||||
import React from 'react' | ||||||
import React, {useEffect, useState, type MutableRefObject} from 'react' | ||||||
import {SearchIcon, XCircleFillIcon, XIcon, FilterRemoveIcon, AlertIcon, ArrowLeftIcon} from '@primer/octicons-react' | ||||||
|
||||||
import type {ButtonProps, TextInputProps, ActionListProps, LinkProps, CheckboxProps} from '../../index' | ||||||
import {Button, IconButton, Heading, Box, Tooltip, TextInput, Spinner, Text, Octicon, Link, Checkbox} from '../../index' | ||||||
import { | ||||||
Button, | ||||||
IconButton, | ||||||
Heading, | ||||||
Box, | ||||||
Tooltip, | ||||||
TextInput, | ||||||
Spinner, | ||||||
Text, | ||||||
Octicon, | ||||||
Link, | ||||||
Checkbox, | ||||||
useFormControlForwardedProps, | ||||||
} from '../../index' | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the hook recently created by @iansan5653 in this PR: #3632. This hook is also being used in the
|
||||||
import {ActionListContainerContext} from '../../ActionList/ActionListContainerContext' | ||||||
import {useSlots} from '../../hooks/useSlots' | ||||||
import {useProvidedRefOrCreate, useId, useAnchoredPosition} from '../../hooks' | ||||||
|
@@ -337,7 +350,27 @@ const Panel: React.FC<SelectPanelProps> = ({ | |||||
} | ||||||
|
||||||
const SelectPanelButton = React.forwardRef<HTMLButtonElement, ButtonProps>((props, anchorRef) => { | ||||||
return <Button ref={anchorRef} {...props} /> | ||||||
const inputProps = useFormControlForwardedProps(props) | ||||||
const [labelText, setLabelText] = useState('') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Followed the pattern suggested in this slack thread: https://github.slack.com/archives/CSGAVNZ19/p1709843761424779?thread_ts=1709836721.563139&cid=CSGAVNZ19 |
||||||
useEffect(() => { | ||||||
const label = document.querySelector(`[for='${inputProps.id}']`) | ||||||
if (label?.textContent) { | ||||||
setLabelText(label.textContent) | ||||||
} | ||||||
}, [inputProps.id]) | ||||||
|
||||||
if (labelText && inputProps.children) { | ||||||
return ( | ||||||
<Button | ||||||
ref={anchorRef} | ||||||
aria-label={`${labelText}, ${(anchorRef as MutableRefObject<HTMLButtonElement>).current.textContent}`} | ||||||
aria-labelledby={inputProps.id} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this implementation is cleaner overall since it doesn't require having a hidden element just to add punctuation and seems more similar to what we do elsewhere in the Primer codebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we flip this, so that the button text content is first?
Suggested change
This way, the contents of the button is announced first, and the label second. We'd want this mainly because the most important content is the button's text content, whereas the label itself would be secondary to that, after it has been read. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure I can go ahead and flip that 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changes are pushed |
||||||
{...inputProps} | ||||||
/> | ||||||
) | ||||||
} else { | ||||||
return <Button ref={anchorRef} {...props} /> | ||||||
} | ||||||
}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This achieves the intended accessible name: The other implementation I tried was return (
<>
{inputProps.id && (
<VisuallyHidden id={`${inputProps.id}--select-panel-button-children`}>{inputProps.children}</VisuallyHidden>
)}
<Button
ref={anchorRef}
{...inputProps}
aria-labelledby={`${inputProps.id} ${inputProps.id}--select-panel-button-children`}
/>
</>
) However I wasn't thrilled about using The solution I ended up implementing is similar to what @TylerJDev suggested: <label id="label1">SelectPanel within FormControl</label>
<button id="label2" aria-labelledby="label1 label2">Choose a tag</button> The only wrinkle is that I had to give the Is there a more elegant solution here, perhaps involving forwarding React refs? Or is this solution satisfactory? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a problem 🤔
I need to check if we can reuse the id that's generated by the FormControl for this purpose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I tried reusing the id that's generated by the FormControl for this purpose it ended up giving it the name: "SelectPanel within FormControl SelectPanel within FormControl" since the label overrides the inner text of the button when linking with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I've been busy with first responder duties. I need to check if this is safe/fine. @TylerJDev can you validate this for me, please? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm curious about where it gave it the name twice? Could we not utilize the ID There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll take a video and sent it over. When I was doing it it was duplicating the text There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh interesting. When it's this order it works aria-labelledby={`${labelId} ${inputProps.id}`} When it's this order it duplicates "SelectPanel within FormControl` aria-labelledby={`${inputProps.id} ${labelId}`} Screen.Recording.2024-03-22.at.1.11.56.PM.movThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright I went ahead and removed |
||||||
|
||||||
const SelectPanelHeader: React.FC<React.PropsWithChildren & {onBack?: () => void}> = ({children, onBack, ...props}) => { | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a 2nd test to ensure the aria-label is still correct even if a button's contents isn't solely text (ie it includes HTML in it)