feat: Update Sana Canvas Popup, Modal and Dialog#4018
Conversation
… feat/update-sana-canvas-popup-dialog-modal-26-06
|
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 due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR adds ChangesButtonGroup subcomponent and story migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
josh-bagwell
changed the title
josh-bagwell
changed the title
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
feat/update-sana-canvas-popup-dialog-modal-26-06
|
| Run status |
|
| Run duration | 02m 24s |
| Commit |
|
| Committer | Josh |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
809
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.63%
|
|
|---|---|
|
|
1525
|
|
|
370
|
Accessibility
99.41%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
68
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
modules/react/popup/lib/PopupButtonGroup.tsx (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
growselector matches all descendant buttons.
& buttonis a descendant combinator, so when aButtonGroupcontains nested interactive content (e.g. the nestedPopup/Modalpatterns inModalWithPopup/WithTooltips), enablinggrowwould also force-width any deeperbuttonelements. Consider scoping to direct children (& > button) if only the group's own buttons should grow.♻️ Optional scoping
grow: { true: { - '& button': { + '& > button': { width: '100%', }, }, },🤖 Prompt for 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. In `@modules/react/popup/lib/PopupButtonGroup.tsx` around lines 41 - 47, The `grow` style in `PopupButtonGroup` is too broad because `& button` targets all descendant buttons, including nested buttons inside popup/modal content. Update the `grow.true` rule in `PopupButtonGroup.tsx` to scope the width styling to the group’s direct button children only, using the relevant selector in the `grow` block so nested interactive content is unaffected.
🤖 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/modal/lib/Modal.tsx`:
- Around line 95-99: The JSDoc for `Modal.ButtonGroup` contains a broken symbol
reference: it points to `modalButtonGroupStencil`, which does not exist, and the
linked `ButtonGroup` reference should also be aligned with the actual
implementation. Update the documentation in `Modal.ButtonGroup` to reference the
existing `popupButtonGroupStencil` used by `Popup.ButtonGroup`, and fix the
corresponding broken `{`@link` ButtonGroup}` reference in `Popup.tsx` so the docs
resolve correctly.
In `@modules/react/modal/stories/examples/ModalWithPopup.tsx`:
- Around line 34-37: The Cancel action inside ModalWithPopup is currently using
Popup.CloseButton, so it only targets the nested popup instead of dismissing the
surrounding modal. Update the button in the ModalWithPopup story to use the
modal’s close/dismiss control (or wire it through the modal’s close handler) and
keep the popup-specific controls only for Popup.ButtonGroup, Popup.CloseButton,
and Popup.Target where they actually manage the popup state.
In `@modules/react/modal/stories/examples/ModalWithPopupRTL.tsx`:
- Around line 35-38: The first visible “Cancel” action in the RTL modal is still
wired to Popup.CloseButton, so it only targets the popup instead of dismissing
the modal. Update the ModalWithPopupRTL story to bind that outer Cancel control
to the modal’s close action (using the modal component’s close/dismiss wiring)
while keeping the popup-specific controls inside Popup.ButtonGroup unchanged.
Use the existing Popup and modal button setup in ModalWithPopupRTL to locate the
affected JSX and switch the Cancel button to the modal-level handler.
In `@modules/react/popup/lib/Popup.tsx`:
- Around line 115-119: The JSDoc for Popup.ButtonGroup incorrectly links to a
non-existent ButtonGroup symbol and should describe PopupButtonGroup as its own
standalone component instead. Update the doc text on PopupButtonGroup in
Popup.tsx to remove the broken {`@link` ButtonGroup} reference while keeping the
valid {`@link` popupButtonGroupStencil} link, and apply the same wording fix to
the duplicated documentation in Dialog.tsx and Modal.tsx.
---
Nitpick comments:
In `@modules/react/popup/lib/PopupButtonGroup.tsx`:
- Around line 41-47: The `grow` style in `PopupButtonGroup` is too broad because
`& button` targets all descendant buttons, including nested buttons inside
popup/modal content. Update the `grow.true` rule in `PopupButtonGroup.tsx` to
scope the width styling to the group’s direct button children only, using the
relevant selector in the `grow` block so nested interactive content is
unaffected.
🪄 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: b46e66e1-4b71-425e-9951-674d7ee0b1d1
📒 Files selected for processing (31)
modules/react/dialog/lib/Dialog.tsxmodules/react/dialog/stories/FallbackTesting.stories.tsxmodules/react/dialog/stories/examples/Basic.tsxmodules/react/dialog/stories/examples/DialogWithFallbackPlacements.tsxmodules/react/dialog/stories/examples/Focus.tsxmodules/react/modal/lib/Modal.tsxmodules/react/modal/stories/examples/Basic.tsxmodules/react/modal/stories/examples/BodyOverflow.tsxmodules/react/modal/stories/examples/CustomFocus.tsxmodules/react/modal/stories/examples/FormModal.tsxmodules/react/modal/stories/examples/FullOverflow.tsxmodules/react/modal/stories/examples/IframeTest.tsxmodules/react/modal/stories/examples/ModalWithPopup.tsxmodules/react/modal/stories/examples/ModalWithPopupRTL.tsxmodules/react/modal/stories/examples/NoTargetRTL.tsxmodules/react/modal/stories/examples/ReturnFocus.tsxmodules/react/modal/stories/examples/StackedModals.tsxmodules/react/modal/stories/examples/WithTooltips.tsxmodules/react/modal/stories/examples/WithoutCloseIcon.tsxmodules/react/popup/lib/Popup.tsxmodules/react/popup/lib/PopupButtonGroup.tsxmodules/react/popup/lib/PopupCard.tsxmodules/react/popup/stories/examples/Basic.tsxmodules/react/popup/stories/examples/FocusRedirect.tsxmodules/react/popup/stories/examples/FocusTrap.tsxmodules/react/popup/stories/examples/InitialFocus.tsxmodules/react/popup/stories/examples/InlinePopup.tsxmodules/react/popup/stories/examples/MultiplePopups.tsxmodules/react/popup/stories/examples/NestedPopups.tsxmodules/react/popup/stories/examples/PopupWithFallbackPlacements.tsxmodules/react/popup/stories/examples/RTL.tsx
josh-bagwell
added
ready for review
williamjstanton
left a comment
williamjstanton
left a comment
There was a problem hiding this comment.
Possible omissions on the upgrade guide? Can we double check on these?
Low: The v16 upgrade guide does not mention the new Popup/Dialog/Modal.ButtonGroup public API or the Popup.Card padding change.
Otherwise, looking good on accessibility side so far.
| }, | ||
| }); | ||
|
|
||
| export const PopupButtonGroup = createSubcomponent('div')({ |
There was a problem hiding this comment.
Medium: Popup.ButtonGroup is documented in JSDoc as a button “group,” but it renders a layout-only div with no group semantics. That may be fine for dialog action rows, but it conflicts with existing Button docs that recommend semantic grouping via ul or fieldset/legend for related buttons. Recommend documenting *.ButtonGroup as visual/layout-only, and showing when to add role="group" plus an accessible name.
There was a problem hiding this comment.
I can update this. Thanks WIlliam!
There was a problem hiding this comment.
When would be a good time to add a role="group" to the group?
Thoughts on the accessible name being "Actions" (i.e. aria-label="Actions").
| boxShadow: system.depth[3], | ||
| minHeight: 0, | ||
| padding: system.legacy.padding.xl, | ||
| padding: system.legacy.padding.xxl, |
There was a problem hiding this comment.
I’d still recommend adding or running a 320px viewport visual/accessibility check because Popup.Card padding increased to xxl, reducing usable content width.
There was a problem hiding this comment.
Let me take a look.
| @@ -198,7 +198,7 @@ describe('Popup', () => { | |||
| // useFocusRedirect hides on Tab only from the last focusable in the popup (Close, Delete, Cancel) | |||
| context('when the "Cancel" button has focus and the tab key is pressed', () => { | |||
There was a problem hiding this comment.
Low: The focus redirect Cypress description/comment is now stale: it says the Cancel button has focus and lists order as Close, Delete, Cancel, but the test focuses Delete after the PR’s reordered DOM. That weakens the accessibility spec around focus order.
|
Do we have padding changed? It looks different in visual tests. We also need to setup sana theme in tests. |
We don't have specific info on smaller sizes but, I have it set to stacking: |
3 participants
Summary
Fixes: #3988
Release Category
Components
Release Note
Updates
Popup, Modal and Dialogto new Sana Canvas styling.BREAKING CHANGES
Visual.
Where Should the Reviewer Start?
/modules/react/dialog/modules/react/modal/modules/react/popupSummary by CodeRabbit
New Features
Dialog.ButtonGroup,Modal.ButtonGroup, andPopup.ButtonGroupfor consistent grouped action-button layouts (with configurable alignment for popups).Style
Tests