-
Notifications
You must be signed in to change notification settings - Fork 615
[SelectPanel] Add notice announcements to SelectPanel #6210
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
Conversation
🦋 Changeset detectedLatest commit: bcaaf37 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 |
Uh oh! @kelsey-myers, at least one image you shared is missing helpful alt text. Check your pull request body to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
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.
Pull Request Overview
This patch adds support for announcing notice messages in SelectPanel
when it opens.
- Imports
announceFromElement
and adds a ref to the notice container - Implements a
useEffect
to clear and announce the notice whenopen && notice
- Adds a test to verify that the notice text is announced
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/react/src/SelectPanel/SelectPanel.tsx | Added noticeRef, import of announceFromElement , and useEffect to announce notices |
packages/react/src/SelectPanel/SelectPanel.test.tsx | New test case to assert notice announcements |
.changeset/smooth-ideas-smile.md | Bumped patch version and changelog entry |
size-limit report 📦
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/384379 |
Failing tests are unrelated 🐸 |
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 great, thank you for this contribution @kelsey-myers! ✨ Left a few comments, let me know what you think!
useEffect(() => { | ||
const announceNotice = async () => { | ||
if (!noticeRef.current) return | ||
const liveRegion = document.querySelector('live-region') |
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.
Would it be possible to utilize the announceText
function here instead?
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.
@TylerJDev I did it this way becausenotice.text
can either be a string or a react element, so I figured announceFromElement
covers both cases? (unless you know of a way we can extract the text from the element)
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.
Makes sense! We can leave it as-is then 😁
@@ -739,7 +757,7 @@ function Panel({ | |||
) : null} | |||
</div> | |||
{notice && ( | |||
<div aria-live="polite" data-variant={notice.variant} className={classes.Notice}> | |||
<div ref={noticeRef} aria-live="polite" data-variant={notice.variant} className={classes.Notice}> |
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.
Since we're utilizing announce
, we can most likely remove the aria-live
here, as it is redundant.
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.
Good point, updated :)
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.
LGTM! ✨
Closes #11748 and #5345
Changelog
New
Rollout strategy
Testing & Reviewing
Merge checklist