feat: Add Sana Canvas Menu Updates#4052
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMenu styling was updated for v16 with new corner-shape, spacing, color, state, and icon rules. Visual fixtures cover focused-disabled states. Upgrade guides document the changes, and Canvas token dependencies were updated to beta.4. ChangesMenu v16 updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
sana-canvas-menu
|
| Run status |
|
| Run duration | 02m 27s |
| Commit |
|
| Committer | Sheelah Brennan |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
809
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.65%
|
|
|---|---|
|
|
1527
|
|
|
371
|
Accessibility
99.37%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
72
|
williamjstanton
left a comment
williamjstanton
left a comment
There was a problem hiding this comment.
This looks really good! I just made a couple of notes about the upgrade guides, but that's all.
|
|
||
| - Padding updated to use `paddingBlock: padding.xs` with `paddingInline: padding.sm` / `padding.md`. | ||
|
|
||
| #### State Style Updates (Menu.Item) |
There was a problem hiding this comment.
Concern: the upgrade guide labels the state table as State Style Updates (Menu.Item) and includes Selected / Selected + Focus rows. In ARIA menu patterns, Menu.Item renders role="menuitem", and aria-selected is not valid for menuitem. Persistent selected state belongs to Menu.Option when used in a listbox pattern, such as Select/Combobox.
Recommendation: rename or split that section so the selected rows are clearly tied to Menu.Option, not Menu.Item. For example:
State Style Updates (Menu.Item and Menu.Option)
I want to avoid implying that consumers (or code-gen tools) should use aria-selected on ARIA menu items.
There was a problem hiding this comment.
Ah good callout - thank you! Ideally we'd separate these out in Figma too as "selected" state is very overloaded otherwise. I'll update this.
There was a problem hiding this comment.
@williamjstanton Thanks again for the review! I have these docs updated in case you want to take a look.
This branch also now has the full set of color token changes updated (selectable menu items' selected state background and text colors are blue, matching Figma), which you can see here:
https://5d854c26ba934e0020f5e98a-uxishmmarl.chromatic.com/?path=/story/components-popups-menu--selectable-menu
|
|
||
| - Padding updated to use `paddingBlock: padding.xs` with `paddingInline: padding.sm` / `padding.md`. | ||
|
|
||
| #### State Style Updates (Menu.Item) |
There was a problem hiding this comment.
My previous comment applies here too.
Also adds a visual test for this.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/react/menu/lib/MenuItem.tsx`:
- Around line 143-144: Disabled selected items lose their selected background
because the disabled hover rule also matches aria-selected items and overrides
the selected styling. In MenuItem, update the disabled-state selector in the
rule containing background: 'none' to remove [aria-selected=true], preserving
the existing selected background for disabled+selected items.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b1dc74d4-816b-474e-a634-dae27b30e54f
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (16)
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/mdx/16.0-UPGRADE-GUIDE.mdxmodules/docs/package.jsonmodules/labs-react/package.jsonmodules/preview-react/package.jsonmodules/react/menu/lib/MenuCard.tsxmodules/react/menu/lib/MenuGroup.tsxmodules/react/menu/lib/MenuItem.tsxmodules/react/menu/lib/MenuList.tsxmodules/react/menu/lib/MenuOption.tsxmodules/react/menu/lib/Submenu.tsxmodules/react/menu/stories/testingVisual.stories.tsxmodules/react/package.jsonmodules/styling-transform/package.jsonmodules/styling/package.jsonpackage.json
Stop the background:none rule from matching aria-selected, which was clearing the selected fill for non-interactive but still- selected listbox options.
2 participants
Summary
Fixes: #3987.
Release Category
Components
Checklist
ready for reviewhas been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)
Summary by CodeRabbit
Summary by CodeRabbit
UI Updates
Documentation
Visual Testing