A11y: improve interactions panel semantics, labels, and run announcements#34109
Closed
anchmelev wants to merge 2 commits into
Closed
A11y: improve interactions panel semantics, labels, and run announcements#34109anchmelev wants to merge 2 commits into
anchmelev wants to merge 2 commits into
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors Interaction and InteractionsPanel for semantic HTML and accessibility: introduces helpers for interaction labels/status, changes Interaction row to a list item with updated ARIA and chevron behavior, adds an id prop, live-region status announcements, aria-busy handling, and a new test suite covering rendering and accessibility states. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
code/core/src/component-testing/components/InteractionsPanel.test.tsx (1)
48-106: Consider adding a test for the collapsed toggle state.The test suite covers
aria-expanded="true"(whenisCollapsed: false), but doesn't verifyaria-expanded="false"when collapsed. This would ensure the toggle correctly reflects both states.💡 Optional test addition
it('labels nested-step toggle with collapsed state', () => { const interactions = getInteractions(CallStates.DONE).map((interaction) => interaction.method === 'step' ? { ...interaction, childCallIds: ['child-call-id'], isCollapsed: true } : interaction ); renderPanel(createProps({ interactions })); const toggle = screen.getByRole('button', { name: 'Expand nested interaction steps for Click button', }); expect(toggle).toHaveAttribute('aria-expanded', 'false'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/component-testing/components/InteractionsPanel.test.tsx` around lines 48 - 106, Add a complementary test to cover the collapsed toggle state: create interactions using getInteractions(CallStates.DONE) and for the interaction with method === 'step' set childCallIds: ['child-call-id'] and isCollapsed: true, then renderPanel(createProps({ interactions })), find the toggle button by its accessible name "Expand nested interaction steps for Click button" and assert toggle has attribute aria-expanded="false"; place this alongside the existing "labels nested-step toggle buttons with action and expanded state" test to ensure both expanded and collapsed states of the toggle (referencing renderPanel, createProps, getInteractions, CallStates, and the toggle button assertion).code/core/src/component-testing/components/InteractionsPanel.tsx (1)
145-156: Consider extracting status announcements to a mapping object.The nested ternary handles all cases correctly, but a mapping object (similar to
StatusTextMappinginStatusBadge.tsx) could improve readability and maintainability.♻️ Optional refactor
+const statusAnnouncementMap: Record<PlayStatus, string> = { + rendering: 'Component test is rendering.', + playing: 'Component test is running.', + errored: 'Component test failed.', + aborted: 'Component test was aborted.', + completed: 'Component test completed successfully.', +}; // In component: -const statusAnnouncement = - status === 'rendering' - ? 'Component test is rendering.' - : status === 'playing' - ? 'Component test is running.' - : status === 'errored' - ? 'Component test failed.' - : status === 'aborted' - ? 'Component test was aborted.' - : hasException - ? 'Component test completed with errors.' - : 'Component test completed successfully.'; +const statusAnnouncement = + status === 'completed' && hasException + ? 'Component test completed with errors.' + : statusAnnouncementMap[status];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/component-testing/components/InteractionsPanel.tsx` around lines 145 - 156, Replace the nested ternary that produces statusAnnouncement with a lookup from a mapping object (e.g., create a const StatusAnnouncementMapping similar to StatusTextMapping in StatusBadge.tsx) keyed by the status values ('rendering','playing','errored','aborted') and then compute statusAnnouncement by checking StatusAnnouncementMapping[status] with a fallback that uses hasException to choose between the error and success messages; update the InteractionsPanel.tsx references to use this mapping so the logic is clearer and easier to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/component-testing/components/InteractionsPanel.tsx`:
- Line 143: The component currently calls React.useId() (headingId) which breaks
in React 17; update InteractionsPanel to accept an optional id prop (e.g., id?:
string on the component props) and replace the React.useId() call with a value
that prefers the caller-provided id and falls back to a client-only generated id
(useRef + useEffect or a lazy random/id generator) so server/React17 rendering
is safe; change references of headingId to use this resolvedId and ensure prop
typing and defaulting are updated accordingly.
---
Nitpick comments:
In `@code/core/src/component-testing/components/InteractionsPanel.test.tsx`:
- Around line 48-106: Add a complementary test to cover the collapsed toggle
state: create interactions using getInteractions(CallStates.DONE) and for the
interaction with method === 'step' set childCallIds: ['child-call-id'] and
isCollapsed: true, then renderPanel(createProps({ interactions })), find the
toggle button by its accessible name "Expand nested interaction steps for Click
button" and assert toggle has attribute aria-expanded="false"; place this
alongside the existing "labels nested-step toggle buttons with action and
expanded state" test to ensure both expanded and collapsed states of the toggle
(referencing renderPanel, createProps, getInteractions, CallStates, and the
toggle button assertion).
In `@code/core/src/component-testing/components/InteractionsPanel.tsx`:
- Around line 145-156: Replace the nested ternary that produces
statusAnnouncement with a lookup from a mapping object (e.g., create a const
StatusAnnouncementMapping similar to StatusTextMapping in StatusBadge.tsx) keyed
by the status values ('rendering','playing','errored','aborted') and then
compute statusAnnouncement by checking StatusAnnouncementMapping[status] with a
fallback that uses hasException to choose between the error and success
messages; update the InteractionsPanel.tsx references to use this mapping so the
logic is clearer and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7af7b841-f82a-49fe-a704-94be864ee971
📒 Files selected for processing (3)
code/core/src/component-testing/components/Interaction.tsxcode/core/src/component-testing/components/InteractionsPanel.test.tsxcode/core/src/component-testing/components/InteractionsPanel.tsx
|
View your CI Pipeline Execution ↗ for commit c4ed70a
☁️ Nx Cloud last updated this comment at |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #31701
What I did
ol+li) instead of non-semantic wrappers.Interaction steps).aria-expanded.rightwhen collapsed,downwhen expanded).aria-busyto the interactions list while tests are rendering/running.code/core/src/component-testing/components/InteractionsPanel.test.tsxChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
cd code && yarn task compile && yarn storybook:uihttp://localhost:6006/?path=/story/core-component-test-basics--stepol/li).Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Tests