Only add the loading id when loading is shown #7347
Conversation
🦋 Changeset detectedLatest commit: ed5f23e 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 |
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
owenniblock
added
the
update snapshots
There was a problem hiding this comment.
Pull request overview
This PR fixes an accessibility issue where the Button component was always including a loading announcement ID in its aria-describedby attribute, even when the button was not in a loading state. The fix ensures that the loading announcement ID is only included when the button is actually loading, matching when the AriaStatus element is rendered in the DOM.
Key changes:
- Conditional construction of
aria-describedbyIDs based on loading state - Simplified inline filtering logic by pre-filtering IDs
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/Button/ButtonBase.tsx | Implements conditional logic to only include loading announcement ID in aria-describedby when loading is true |
| .changeset/ripe-baths-rush.md | Adds changeset documenting this patch-level fix for aria-describedby behavior |
github-actions
bot
removed
the
update snapshots
| ) | ||
| const buttonNode = container.getByRole('button') | ||
|
|
||
| fireEvent.click(buttonNode) |
There was a problem hiding this comment.
The test component shows the loading element after the button has been clicked.
There was a problem hiding this comment.
Sounds sensible!
I'd recommend running the integration tests just to make sure there isn't a test in dotcom that expects aria-describedby to always exist
|
@siddharthkp looks like there's an issue with this test: https://github.com/github/github-ui/blob/main/packages/mergebox/components/sections/__tests__/ConflictsSection.test.tsx#L600 - although looking at the logic, I can't see why it's failing 🤔 |
Same, reading the test, it shouldn't fail because of this PR. I'd recommend running it once in codespaces as well for the branch and for main, just to be sure it's related. |
|
OK - it's related. It looks like the waitFor behaves differently when the Whereas the check when there is no The fix is to change the assertion so it's not doing a count (which seems pretty likely to be flaky anyway). I see that this has already been done in another test in the same file Testing with a change on the validation PR. How would we normally handle changes like this? I could cherrypick the fix into github before we merge my original primer react PR since it'll still pass for the old version of Primer? |
|
@siddharthkp there were some legitimate test failures as well which I've added fixes for on that PR. Assuming the PR is green I'll create a clean dotcom PR so we can get this PR moving 😄 |
We don't have any rules around that (yet?). I prefer to do exactly what you said, prepare github for the change and then make the primer change. |
|
@siddharthkp integration test results are all green 🎉 Think this is ready to go 😄 |
Amazing! I see is also merged https://github.com/github/github-ui/pull/9052. I've already approved the PR, so you can merge whenever you'd like -- 
I see the required checks are still waiting and won't let you merge. That's because the integration PR in github-ui does not have the latest commit from this primer/react PR, which is a bit annoying. I'll run it again so that it can pull in the latest commit and let you through |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/9154 |
Done! Merge away! |
|
Yay, thanks @siddharthkp - merging 🎉 |
Closes https://github.com/github/copilot-issues-xp/issues/309
Changelog
New
Changed
Only include the id for the loading state element when the component is in the loading state.
Removed
Rollout strategy
Testing & Reviewing
Testing with storybook
Merge checklist