Repository navigation
fix(fuselage): remove unsupported aria-selected from MenuOption - #42885
mantrapatel05 wants to merge 1 commit into
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: d34cac9 The changes in this PR will be included in the next version bump. 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 💥 An error occurred when fetching the changed packages and changesets in this PR |
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
WalkthroughMenuOption no longer renders ChangesMenuOption ARIA behavior
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~5 minutes Change: Bug fix · Severity of issue fixed: Low Suggested labels: Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely identifies the main change: removing the unsupported aria-selected attribute from MenuOption. |
| Linked Issues check | |
Issue #42805 requires omitting unsupported aria-selected for menuitem, menuitemcheckbox, and menuitemradio. MenuOption.tsx removes the attribute. The selected prop still adds `rcx-option--… |
| Out of Scope Changes check | |
The diff changes only the unsupported attribute and adds focused MenuOption tests. The tests directly verify the behavior required by issue #42805. No unrelated implementation or test changes are pr… |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
Full details: Linked Issues check
-
Explanation
Issue
#42805requires omitting unsupportedaria-selectedformenuitem,menuitemcheckbox, andmenuitemradio.MenuOption.tsxremoves the attribute. Theselectedprop still addsrcx-option--selected. The new tests cover all three roles, selected styling, and an axe check for a checked menu item.
Full details: Out of Scope Changes check
-
Explanation
The diff changes only the unsupported attribute and adds focused
MenuOptiontests. The tests directly verify the behavior required by issue#42805. No unrelated implementation or test changes are present.
Full details: Docstring Coverage
-
Explanation
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Errors were encountered while retrieving linked issues.
Errors: 1
-
- JIRA integration encountered authorization issues. Please disconnect and reconnect the integration in the CodeRabbit UI.
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 help to get the list of available commands.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/fuselage/src/components/Menu/MenuOption.spec.tsx">
<violation number="1" location="packages/fuselage/src/components/Menu/MenuOption.spec.tsx:6">
P2: This spec tests `MenuOption`, which nothing renders anymore. `Menu` renders items through `MenuItem` → `Item`, and `MenuOption` is only imported for its types. The test therefore gives no coverage of the menu items users actually get, and the `aria-selected` line it guards is in unreachable code. Either delete `MenuOption` and this spec, or move the regression check to `Menu`/`MenuItem` so it exercises the real render path.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
| import MenuOption from './MenuOption'; | ||
| import { render } from '../../testing'; | ||
|
|
||
| describe('[MenuOption Component]', () => { |
There was a problem hiding this comment.
P2: This spec tests MenuOption, which nothing renders anymore. Menu renders items through MenuItem → Item, and MenuOption is only imported for its types. The test therefore gives no coverage of the menu items users actually get, and the aria-selected line it guards is in unreachable code. Either delete MenuOption and this spec, or move the regression check to Menu/MenuItem so it exercises the real render path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/fuselage/src/components/Menu/MenuOption.spec.tsx, line 6:
<comment>This spec tests `MenuOption`, which nothing renders anymore. `Menu` renders items through `MenuItem` → `Item`, and `MenuOption` is only imported for its types. The test therefore gives no coverage of the menu items users actually get, and the `aria-selected` line it guards is in unreachable code. Either delete `MenuOption` and this spec, or move the regression check to `Menu`/`MenuItem` so it exercises the real render path.</comment>
<file context>
@@ -0,0 +1,42 @@
+import MenuOption from './MenuOption';
+import { render } from '../../testing';
+
+describe('[MenuOption Component]', () => {
+ it.each(['menuitem', 'menuitemcheckbox', 'menuitemradio'] as const)('should not set aria-selected on %s', (role) => {
+ const { getByRole } = render(
</file context>
There was a problem hiding this comment.
Fair point — MenuOption isn’t used by the current menu anymore since #42818. I kept this PR focused on the fix requested in #42805. @tassoevan, would you prefer I leave it as is or remove MenuOption and move its shared types? Happy to make whichever change you prefer.
Proposed changes
MenuOptionalways setaria-selectedon its element. But menu items (menuitem,menuitemcheckbox,menuitemradio) don't support that attribute, so every item got an invalidaria-selected="false"and axe flagged it.I removed that line. Checkable items already use
aria-checked, and theselectedclass still works the same, so nothing changes visually.Also added a small test file for it. It fails with the old code and passes with the fix.
Heads up: since #42818,
MenurendersItem, soMenuOptionis only used for its types now. Happy to delete it instead if you'd prefer.Issue(s)
Closes #42805
Steps to test or reproduce
Run
yarn jest src/components/Menuinpackages/fuselage.Further comments
Only touched the attribute, nothing else.
Summary by CodeRabbit
aria-selected; theselectedprop continues to apply the selected styling.