feat: Update Sana Canvas Popup, Modal and Dialog#4018
Conversation
… feat/update-sana-canvas-popup-dialog-modal-26-06
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a shared ChangesButtonGroup subcomponent and story migration
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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 29s |
| 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.65%
|
|
|---|---|
|
|
1523
|
|
|
370
|
Accessibility
99.47%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
72
|
![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").
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").
It's a good question. As mentioned already, it's fine without in most situations of a dialog with 2 actions. The aria-label string is always going to be contextual based on the design situation.
I probably should spend some research time brainstorming situations where programmatic grouping is valuable and contribute examples separately. The classic example is always a form that requires you to fill out both a Shipping address and a Billing address. (Fields must necessarily have identical labels, with different group contexts. )
| 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: |
|
Overall PR looks good, but I think we will need to finalize our tokens change strategy before. This PR can request additional changes to how to add theme (if we finally decide to use that for v16) to popups, so Sana styles are consumed by popups. |
RayRedGoose
added
approved
There was a problem hiding this comment.
nit: Good to remove the unused Flex import?
| @@ -198,7 +198,7 @@ describe('Popup', () => { | |||
| // useFocusRedirect hides on Tab only from the last focusable in the popup (Close, Delete, Cancel) | |||
There was a problem hiding this comment.
nit: Do we want to update this comment and the test description below now that the "Delete" button is the last focusable element?
| import {TextInput} from '@workday/canvas-kit-react/text-input'; | ||
| import {system} from '@workday/canvas-tokens-web'; | ||
|
|
||
| export const CustomFocus = () => { |
There was a problem hiding this comment.
Looks like the alignment of the text input and buttons is off now vs v15. I assume we want the buttons and text input's ends to align vertically? Something to confirm with design.
v15:
| padding: system.padding.xs, | ||
| }); | ||
|
|
||
| export const FormModal = () => { |
There was a problem hiding this comment.
Seeing the same alignment issue in this form + the buttons as well:
| <p>Are you sure you want to delete the item?</p> | ||
| <Flex cs={{gap: system.gap.md}}> | ||
| <Modal.CloseButton as={DeleteButton}>Delete</Modal.CloseButton> | ||
| <Modal.ButtonGroup> |
There was a problem hiding this comment.
Same alignment question for this story:
| }, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
this was causing an aggressive border radius which we no longer need
| Caution: Describe type has been deprecated{' '} | ||
| </InformationHighlight.Heading> | ||
| <InformationHighlight.Body> | ||
| <InformationHighlight.Body cs={{'p': {display: 'inline-block'}}}> |
There was a problem hiding this comment.
aligns the status indicators nicely in storybook
5 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
Dialog.ButtonGroup,Modal.ButtonGroup, andPopup.ButtonGroupfor consistent grouped action-button layouts, including alignment options for popups.