-
Notifications
You must be signed in to change notification settings - Fork 538
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
refactor(live-region): update live region helpers to match ADR #4673
Conversation
🦋 Changeset detectedLatest commit: 813845c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
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 looks great! ❤️ Thank you for all your work on this! I think aligning the defaults with default behavior of ARIA is safe to start with, and we could revisit defaults if we need to.
Do you ideas around how we might validate that people are using the API as intended?
@khiga8 love this question, thanks for asking. I definitely want to have a chart over in the Primer Query dashboard not only for adoption but to see what props are being used. My hope is that based on this (and usage within PRC) we get a better idea of what is needed, whether that's lint rules, getting rid of props, or something to help with common gotchas. |
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.
Scanned the code and checked out the story changes. Looks pretty good 👍🏻
@@ -604,7 +604,8 @@ const SelectPanelSecondaryAction: React.FC<SelectPanelSecondaryActionProps> = ({ | |||
|
|||
const SelectPanelLoading = ({children = 'Fetching items...'}: React.PropsWithChildren) => { | |||
return ( | |||
<Status | |||
<AriaStatus | |||
announceOnShow |
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.
Stupid question, are there styles attached to <Status>
and is that different than <AriaStatus>
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.
@jonrohan no styles, just behavior for now 👀 They're both setup as a Box
though internally 😅
Update our live region helpers to match the feedback and decisions from the ADR: #4611
Changelog
New
hidden
toAnnounce
,AriaStatus
, andAriaAlert
so that messages are not announced when the container is hiddendelayMs
toAnnounce
,AriaStatus
, andAriaAlert
to specify the delay before making an announcementAnnounce
,AriaStatus
, andAriaAlert
as experimental componentsChanged
Removed
Rollout strategy