-
Notifications
You must be signed in to change notification settings - Fork 91
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
fix(forms): add message id to input aria-describedby attribute #1389
fix(forms): add message id to input aria-describedby attribute #1389
Conversation
@@ -42,7 +42,7 @@ export const Input = React.forwardRef<HTMLInputElement, IInputProps>( | |||
} | |||
|
|||
if (fieldContext) { | |||
combinedProps = fieldContext.getInputProps(combinedProps, { isDescribed: true }); |
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.
I found that the isDescribed
option is true
on multiple input components including Input
. I found it was safe to remove after getInputProps
was given a default value for the second function parameter. Are there any concerns with removing?
setHint: (hintPresent: boolean) => setHasHint(hintPresent), | ||
hasHint | ||
hasHint, | ||
setHasHint, |
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.
setHint
was renamed to setHasHint
for consistency and clarity in its function, the new setHasMessage
also follows this naming pattern.
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.
looks good.
@@ -42,8 +54,8 @@ export const Message = React.forwardRef<HTMLDivElement, IMessageProps>( | |||
|
|||
let combinedProps = { validation, validationLabel, ...props }; | |||
|
|||
if (fieldContext) { | |||
combinedProps = fieldContext.getMessageProps(combinedProps); | |||
if (typeof getMessageProps === 'function') { |
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.
This seems indicative of poor typing within the context, but might not be worth addressing here.
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.
I've cleaned this up and removed the typeof
check, I was mistakenly thinking the Field context could be misused (however it is not publicly exported).
Description
This PR adds the
Message
component ID in thearia-describedby
attribute for a givenInput
(Select
,Radio
, etc) component within aField
.Detail
There was an update made to
@zendeskgarden/container-field
v2.1.0 which associates theMessage
component ID to thearia-describedby
attribute of a givenInput
when it is present. Thearia-describedby
attribute is a combination of theHint
andMessage
component IDs, however the ID of either of those components will not be added if they are not present as children of theField
component.Cross browser caveats
In Safari (using VoiceOver), a concatenated list of text node IDs are not all read, only the first item is. However, other browsers behave as expected.
Checklist
design updates are Garden Designer approved (add thedesigner as a reviewer)
demo is up-to-date (yarn start
)renders as expected with reversed (RTL) directionrenders as expected with Bedrock CSS (?bedrock
)