feat: Update Sana Canvas Buttons#4000
Conversation
… feat/update-buttons-sana-canvas-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 updates shared button styling tokens, adds inverse DeleteButton support with story and docs coverage, changes ChangesButton styling and inverse DeleteButton
Upgrade guide documentation
Segmented control sizing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![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
🤖 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/button/lib/DeleteButton.tsx`:
- Around line 14-19: The PR objective specifies a `secondary` variant for
DeleteButton, but the current implementation uses `inverse` across all files,
creating a mismatch between intent and implementation. Align all layers to use
the intended variant name consistently: in
modules/react/button/lib/DeleteButton.tsx lines 14-19, change the
DeleteButtonProps.variant type union from 'inverse' to the intended variant
(e.g., 'secondary'); in modules/react/button/lib/DeleteButton.tsx lines 80-119,
update the modifier block logic to use the intended variant key instead of
'inverse'; in modules/react/button/stories/button/examples/DeleteInverse.tsx
lines 15-24, rename the example component to reflect the intended variant name;
in modules/react/button/stories/button/Button.stories.tsx lines 6-7, update the
import statement to use the renamed example component; in
modules/react/button/stories/button/Button.stories.tsx lines 46-48, update both
the story export name and render target to reference the intended variant; in
modules/react/button/stories/button/Button.mdx lines 12-13, update the import to
reference the renamed example; in modules/react/button/stories/button/Button.mdx
lines 86-90, update all prose references and example blocks to use the intended
variant guidance; and in
modules/react/button/stories/visual-testing/DeleteButton.stories.tsx lines
30-33, update the permutation values and labels to use the intended variant
contract throughout.
- Line 142: The deleteButtonStencil call on line 142 in DeleteButton.tsx is
missing the grow parameter, while PrimaryButton and SecondaryButton both pass it
through to their respective stencil calls. Since deleteButtonStencil extends
buttonStencil which defines the grow modifier, omitting it causes the grow
styling to be silently dropped. Add the grow parameter to the
deleteButtonStencil function call on line 142, passing it alongside the variant
and size parameters to maintain consistency with the other button components.
In `@modules/react/button/lib/SecondaryButton.tsx`:
- Around line 27-33: The SecondaryButton's default state at lines 27-33 uses a
transparent border which relies too heavily on the hover state's border color
change for visual differentiation, creating insufficient perceivable change for
low-vision users. Enhance the default state affordance by either adding a
non-transparent subtle border color, introducing a subtle background shift, or
applying another non-color visual cue to the default state styling. Ensure the
hover state styling (lines 39-43) maintains sufficient contrast and visual
distinction from the enhanced default state. After making these changes,
validate the contrast ratios and state distinguishability during the visual
accessibility review to confirm adequate perceivable differentiation exists
between default and hover states.
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Around line 48-57: The TertiaryButton component in the hover and active state
blocks still includes textDecoration: 'underline', which conflicts with the v16
behavior change objective to remove underlines in those states. Remove the
textDecoration: 'underline' line from both the '&:hover, &.hover' block and the
'&:active, &.active' block to align with the intended styling update.
🪄 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: a786b8ad-b7b5-4af3-821e-6612dabc325d
📒 Files selected for processing (8)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/DeleteButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsxmodules/react/button/stories/button/Button.mdxmodules/react/button/stories/button/Button.stories.tsxmodules/react/button/stories/button/examples/DeleteInverse.tsxmodules/react/button/stories/visual-testing/DeleteButton.stories.tsx
| export interface DeleteButtonProps extends ButtonProps { | ||
| /** | ||
| * Variant has an option for `inverse` which will inverse the styling | ||
| */ | ||
| variant?: 'inverse'; | ||
| } |
There was a problem hiding this comment.
Variant contract appears misaligned with the stated PR objective (secondary vs inverse).
The PR objective states DeleteButton should add a secondary variant, but these changes introduce inverse across API, styling, docs, stories, and visual permutations. Please confirm intended product contract, then align all layers consistently.
modules/react/button/lib/DeleteButton.tsx#L14-L19: UpdateDeleteButtonProps.variantunion to the intended variant(s).modules/react/button/lib/DeleteButton.tsx#L80-L119: Rename/rework modifier block to the intended variant key and token behavior.modules/react/button/stories/button/examples/DeleteInverse.tsx#L15-L24: Rename/replace example component to match the intended variant.modules/react/button/stories/button/Button.stories.tsx#L6-L7: Update import to the intended example name.modules/react/button/stories/button/Button.stories.tsx#L46-L48: Update exported story name/render target to intended variant.modules/react/button/stories/button/Button.mdx#L12-L13: Update MDX import for intended variant example.modules/react/button/stories/button/Button.mdx#L86-L90: Update prose/example block to intended variant guidance.modules/react/button/stories/visual-testing/DeleteButton.stories.tsx#L30-L33: Update permutation values/labels to intended variant contract.
📍 Affects 5 files
modules/react/button/lib/DeleteButton.tsx#L14-L19(this comment)modules/react/button/lib/DeleteButton.tsx#L80-L119modules/react/button/stories/button/examples/DeleteInverse.tsx#L15-L24modules/react/button/stories/button/Button.stories.tsx#L6-L7modules/react/button/stories/button/Button.stories.tsx#L46-L48modules/react/button/stories/button/Button.mdx#L12-L13modules/react/button/stories/button/Button.mdx#L86-L90modules/react/button/stories/visual-testing/DeleteButton.stories.tsx#L30-L33
🤖 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/button/lib/DeleteButton.tsx` around lines 14 - 19, The PR
objective specifies a `secondary` variant for DeleteButton, but the current
implementation uses `inverse` across all files, creating a mismatch between
intent and implementation. Align all layers to use the intended variant name
consistently: in modules/react/button/lib/DeleteButton.tsx lines 14-19, change
the DeleteButtonProps.variant type union from 'inverse' to the intended variant
(e.g., 'secondary'); in modules/react/button/lib/DeleteButton.tsx lines 80-119,
update the modifier block logic to use the intended variant key instead of
'inverse'; in modules/react/button/stories/button/examples/DeleteInverse.tsx
lines 15-24, rename the example component to reflect the intended variant name;
in modules/react/button/stories/button/Button.stories.tsx lines 6-7, update the
import statement to use the renamed example component; in
modules/react/button/stories/button/Button.stories.tsx lines 46-48, update both
the story export name and render target to reference the intended variant; in
modules/react/button/stories/button/Button.mdx lines 12-13, update the import to
reference the renamed example; in modules/react/button/stories/button/Button.mdx
lines 86-90, update all prose references and example blocks to use the intended
variant guidance; and in
modules/react/button/stories/visual-testing/DeleteButton.stories.tsx lines
30-33, update the permutation values and labels to use the intended variant
contract throughout.
| grow={grow} | ||
| iconPosition={iconPosition} | ||
| cs={[deleteButtonStencil({size, iconPosition}), cs]} | ||
| cs={[deleteButtonStencil({variant, size, iconPosition}), cs]} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether `grow` is a stencil modifier in button base and compare call-site parity.
set -euo pipefail
echo "== Find grow-related stencil modifier definitions =="
rg -n -C3 '\bgrow\b' modules/react/button/lib/BaseButton.tsx modules/react/button/lib/Button.tsx
echo
echo "== Compare stencil invocation patterns across button components =="
rg -n -C2 'Stencil\(\{[^}]*grow|deleteButtonStencil\(\{|secondaryButtonStencil\(\{|tertiaryButtonStencil\(\{' modules/react/button/libRepository: Workday/canvas-kit
Length of output: 3625
🏁 Script executed:
# Check the deleteButtonStencil definition and its modifier structure
rg -n -A30 'export.*deleteButtonStencil|const deleteButtonStencil' modules/react/button/lib/DeleteButton.tsx | head -50Repository: Workday/canvas-kit
Length of output: 1464
Pass grow into deleteButtonStencil to keep variant parity with other buttons.
On line 142, DeleteButton omits grow in the stencil call while PrimaryButton and SecondaryButton pass it through. Since deleteButtonStencil extends buttonStencil (which defines the grow modifier), this inconsistency silently drops grow styling for DeleteButton.
Suggested fix
- cs={[deleteButtonStencil({variant, size, iconPosition}), cs]}
+ cs={[deleteButtonStencil({variant, size, iconPosition, grow}), cs]}🤖 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/button/lib/DeleteButton.tsx` at line 142, The
deleteButtonStencil call on line 142 in DeleteButton.tsx is missing the grow
parameter, while PrimaryButton and SecondaryButton both pass it through to their
respective stencil calls. Since deleteButtonStencil extends buttonStencil which
defines the grow modifier, omitting it causes the grow styling to be silently
dropped. Add the grow parameter to the deleteButtonStencil function call on line
142, passing it alongside the variant and size parameters to maintain
consistency with the other button components.
josh-bagwell
changed the title
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
feat/update-buttons-sana-canvas-26-06
|
| Run status |
|
| Run duration | 02m 30s |
| 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.51%
|
|
|---|---|
|
|
1541
|
|
|
371
|
Accessibility
99.41%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
68
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/react/button/lib/TertiaryButton.tsx (1)
204-214:⚠️ Potential issue | 🟡 Minor
iconPositionis not derived before being passed to the stencil, causing compound modifiers to not apply.When a consumer renders
<TertiaryButton icon={someIcon}>Label</TertiaryButton>without an expliciticonPosition, the undefined value is passed directly totertiaryButtonStencil. Although theButtoncomponent internally derivesbaseIconPosition = 'start'(for icon + text), that derived value is never used by the stencil, which expects explicit'start','end', or'only'values to match its compound modifiers (lines 98-179).The base
ButtonStencilhas identical compound modifier patterns but with different padding values. SincetertiaryButtonStencilpadding does not apply wheniconPositionis undefined, buttons with icon and label will use only the base button padding (e.g.,md/smfor medium size with 'start') instead of the tertiary-specific padding (e.g.,xs/sm). This creates a visual inconsistency.Derive
baseIconPositioninTertiaryButtonbefore passing it to the stencil, matching the logic inButton.tsx(lines 50-56):const baseIconPosition = iconPosition ? iconPosition : icon ? children ? 'start' : 'only' : undefined; // Then pass baseIconPosition to stencil instead of iconPosition🤖 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/button/lib/TertiaryButton.tsx` around lines 204 - 214, The iconPosition parameter is being passed directly to tertiaryButtonStencil without deriving its value first, causing compound modifiers to not apply when iconPosition is undefined. Derive a baseIconPosition variable in TertiaryButton using the same logic from Button.tsx (lines 50-56): check if iconPosition is explicitly provided, otherwise check if icon exists and whether children exist to determine if the position should be 'start' (icon with text), 'only' (icon without text), or undefined. Then pass this derived baseIconPosition to tertiaryButtonStencil instead of the raw iconPosition parameter in the stencil call around line 204-214.
🧹 Nitpick comments (1)
modules/react/button/lib/TertiaryButton.tsx (1)
3-3: 💤 Low valueUnused import:
px2remis not referenced in this file.The
px2remutility is imported but not used anywhere in the component. Consider removing it to keep imports clean.-import {createStencil, cssVar, px2rem} from '`@workday/canvas-kit-styling`'; +import {createStencil, cssVar} from '`@workday/canvas-kit-styling`';🤖 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/button/lib/TertiaryButton.tsx` at line 3, Remove the unused `px2rem` import from the import statement at the top of the TertiaryButton.tsx file. The import statement should only include `createStencil` and `cssVar` from '`@workday/canvas-kit-styling`', since `px2rem` is not used anywhere in the component.
🤖 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.
Outside diff comments:
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Around line 204-214: The iconPosition parameter is being passed directly to
tertiaryButtonStencil without deriving its value first, causing compound
modifiers to not apply when iconPosition is undefined. Derive a baseIconPosition
variable in TertiaryButton using the same logic from Button.tsx (lines 50-56):
check if iconPosition is explicitly provided, otherwise check if icon exists and
whether children exist to determine if the position should be 'start' (icon with
text), 'only' (icon without text), or undefined. Then pass this derived
baseIconPosition to tertiaryButtonStencil instead of the raw iconPosition
parameter in the stencil call around line 204-214.
---
Nitpick comments:
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Line 3: Remove the unused `px2rem` import from the import statement at the top
of the TertiaryButton.tsx file. The import statement should only include
`createStencil` and `cssVar` from '`@workday/canvas-kit-styling`', since `px2rem`
is not used anywhere in the component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0540207b-8d15-4719-9b1d-15792a8765c5
📒 Files selected for processing (3)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/react/button/lib/SecondaryButton.tsx
- modules/react/button/lib/BaseButton.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
modules/react/button/lib/SecondaryButton.tsx (1)
3-3: ⚡ Quick winRemove unused
colorSpaceimport.The
colorSpaceimport is no longer used after removing thecolorSpace.darken()calls from hover/active states.♻️ Suggested fix
-import {colorSpace, createStencil, cssVar} from '`@workday/canvas-kit-styling`'; +import {createStencil, cssVar} from '`@workday/canvas-kit-styling`';🤖 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/button/lib/SecondaryButton.tsx` at line 3, Remove the unused `colorSpace` import from the import statement in SecondaryButton.tsx. The `colorSpace` module is no longer referenced in the code after the `colorSpace.darken()` calls were removed from the hover and active states. Keep the other imports `createStencil` and `cssVar` which are still in use. Simply delete `colorSpace` from the destructured import list from '`@workday/canvas-kit-styling`'.modules/react/button/lib/TertiaryButton.tsx (1)
1-1: ⚡ Quick winRemove unused
focusRingimport.The
focusRingimport is no longer used after refactoring focus styles to use explicitboxShadowInner/boxShadowOutervalues.♻️ Suggested fix
-import {createComponent, focusRing} from '`@workday/canvas-kit-react/common`'; +import {createComponent} from '`@workday/canvas-kit-react/common`';🤖 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/button/lib/TertiaryButton.tsx` at line 1, Remove the unused `focusRing` import from the import statement at the top of TertiaryButton.tsx. The `focusRing` symbol is no longer needed since the focus styles have been refactored to use explicit `boxShadowInner` and `boxShadowOuter` values instead. Keep the `createComponent` import and remove only `focusRing` from the destructured imports coming from '`@workday/canvas-kit-react/common`'.
🤖 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.
Nitpick comments:
In `@modules/react/button/lib/SecondaryButton.tsx`:
- Line 3: Remove the unused `colorSpace` import from the import statement in
SecondaryButton.tsx. The `colorSpace` module is no longer referenced in the code
after the `colorSpace.darken()` calls were removed from the hover and active
states. Keep the other imports `createStencil` and `cssVar` which are still in
use. Simply delete `colorSpace` from the destructured import list from
'`@workday/canvas-kit-styling`'.
In `@modules/react/button/lib/TertiaryButton.tsx`:
- Line 1: Remove the unused `focusRing` import from the import statement at the
top of TertiaryButton.tsx. The `focusRing` symbol is no longer needed since the
focus styles have been refactored to use explicit `boxShadowInner` and
`boxShadowOuter` values instead. Keep the `createComponent` import and remove
only `focusRing` from the destructured imports coming from
'`@workday/canvas-kit-react/common`'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e259c913-e45b-4c79-a787-d8e8f4692428
📒 Files selected for processing (6)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/DeleteButton.tsxmodules/react/button/lib/PrimaryButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsxmodules/styling/lib/colorSpace.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/react/button/lib/BaseButton.tsx
- modules/react/button/lib/DeleteButton.tsx
… feat/update-buttons-sana-canvas-26-06
… feat/update-buttons-sana-canvas-26-06
…sh-bagwell/canvas-kit into feat/update-buttons-sana-canvas-26-06
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/styling/spec/colorSpace.spec.ts (1)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit
surfaceType: 'inverse'assertions for hover/pressed.Line 29 and Line 54 currently validate only the
defaultsurface branch. SincecolorSpacenow branches onsurfaceType, add inverse-path expectations too to prevent token-regression gaps.Also applies to: 54-63
🤖 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/styling/spec/colorSpace.spec.ts` around lines 29 - 41, The hover and pressed specs in colorSpace.spec.ts only cover the default surface branch, so add explicit assertions for the inverse surface path as well. Update the existing colorSpace.hover and colorSpace.pressed test cases to include a `surfaceType: 'inverse'` variant and verify the expected token strings for that branch, using the same `colorSpace` helper and token placeholders already in the spec.
🤖 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/button/lib/PrimaryButton.tsx`:
- Around line 88-91: The hover and active state colorSpace calls in
PrimaryButton are using the default surface overlay path instead of the inverse
variant, which can produce incorrect overlays for inverse buttons. Update the
affected colorSpace.hover and colorSpace.active usages in PrimaryButton to pass
surfaceType: 'inverse' alongside the existing color and colorType arguments so
the inverse state styling resolves correctly.
---
Nitpick comments:
In `@modules/styling/spec/colorSpace.spec.ts`:
- Around line 29-41: The hover and pressed specs in colorSpace.spec.ts only
cover the default surface branch, so add explicit assertions for the inverse
surface path as well. Update the existing colorSpace.hover and
colorSpace.pressed test cases to include a `surfaceType: 'inverse'` variant and
verify the expected token strings for that branch, using the same `colorSpace`
helper and token placeholders already in the spec.
🪄 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: fe106b54-766a-4ed4-bc99-4825bf6a70d3
📒 Files selected for processing (7)
modules/react/button/lib/BaseButton.tsxmodules/react/button/lib/DeleteButton.tsxmodules/react/button/lib/PrimaryButton.tsxmodules/react/button/lib/SecondaryButton.tsxmodules/react/button/lib/TertiaryButton.tsxmodules/styling/lib/colorSpace.tsxmodules/styling/spec/colorSpace.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/styling/lib/colorSpace.tsx
- modules/react/button/lib/DeleteButton.tsx
- modules/react/button/lib/SecondaryButton.tsx
- modules/react/button/lib/TertiaryButton.tsx
| [buttonStencil.vars.background]: colorSpace.hover({ | ||
| color: cssVar(brand.action.lightest, system.legacy.color.surface.inverse), | ||
| mixinColor: system.legacy.color.accent.overlay.mixin, | ||
| mixinValue: system.legacy.opacity.accent.hover, | ||
| fallback: cssVar(brand.action.lightest, system.legacy.color.surface.inverse), | ||
| colorType: 'surface', |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass surfaceType: 'inverse' in inverse hover/active colorSpace calls.
Line 88 and Line 110 currently use the default surface overlay branch, so inverse buttons can resolve incorrect state overlays.
Suggested fix
'&:hover, &.hover': {
[buttonStencil.vars.background]: colorSpace.hover({
color: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
fallback: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
colorType: 'surface',
+ surfaceType: 'inverse',
}), '&:active, &.active': {
[buttonStencil.vars.background]: colorSpace.pressed({
color: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
fallback: cssVar(brand.action.lightest, system.legacy.color.surface.inverse),
colorType: 'surface',
+ surfaceType: 'inverse',
}),Also applies to: 110-113
🤖 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/button/lib/PrimaryButton.tsx` around lines 88 - 91, The hover
and active state colorSpace calls in PrimaryButton are using the default surface
overlay path instead of the inverse variant, which can produce incorrect
overlays for inverse buttons. Update the affected colorSpace.hover and
colorSpace.active usages in PrimaryButton to pass surfaceType: 'inverse'
alongside the existing color and colorType arguments so the inverse state
styling resolves correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/styling/lib/colorSpace.tsx (1)
83-98: 🎯 Functional Correctness | 🔴 CriticalCorrect implementation and tests must be updated to address mismatch.
The
InteractiveStatePropsinterface correctly removedsurfaceType, but the function implementation inmodules/styling/lib/colorSpace.tsxnow resolves tokens tosystem.legacy.color[colorType].overlay.mixin(a single generic color) rather than the specificsurfaceType-suffixed tokens expected by tests.This results in:
- Token Regression: The CSS generated no longer differentiates between
defaultandinversesurface variants, collapsing them to--cnvs-sys-color-surface-overlay-mixin.- Failing Tests:
modules/styling/spec/colorSpace.spec.ts(lines 33-39, 58-64) explicitly passes the undefinedsurfaceTypeproperty and expects the old token structure (e.g.,--cnvs-sys-color-surface-overlay-hover-default).- Type Safety: Callers passing
surfaceTypewill encounter TypeScript errors ifInteractiveStatePropsremains the interface.The implementation logic should either be updated to support the specific token suffixes if distinct behavior is required, or the tests/documentation must be updated to remove
surfaceTypearguments and update expected output strings to match the genericoverlay-mixintoken being used.Comparison
Current Implementation Output:
color-mix(in srgb, var(--cnvs-sys-color-brand-accent-primary, var(--cnvs-brand-primary-base)) , var(--cnvs-sys-color-surface-overlay-mixin, var(black)) calc(var(--cnvs-sys-opacity-surface-hover, 0) * 100%))Test File Expectation (lines 38, 63):
// Expects distinct hover vs pressed suffixes and default suffix var(--cnvs-sys-color-surface-overlay-hover-default, black)🤖 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/styling/lib/colorSpace.tsx` around lines 83 - 98, The interactive color token resolution in colorSpace.tsx no longer matches the spec expectations because it now uses the generic surface overlay mixin token instead of surfaceType-specific suffixes. Update the implementation around the InteractiveStateProps-driven color mix logic to either restore distinct default/inverse surface token handling or align the tests and docs with the new generic overlay-mixin behavior. Also update modules/styling/spec/colorSpace.spec.ts to remove surfaceType usage and assert the new resolved token strings consistently.
🤖 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/styling/lib/colorSpace.tsx`:
- Around line 113-120: The hover and pressed state color helpers are both using
the wrong overlay token path, so update the state-specific mixin selection in
the interactive color helpers in colorSpace.tsx: have hover resolve from
system.legacy.color[colorType].overlay.hover and pressed resolve from
system.legacy.color[colorType].overlay.pressed instead of overlay.mixin, keeping
the existing darken call structure intact so the generated CSS variables match
the spec and the hover/pressed token names remain distinct.
---
Outside diff comments:
In `@modules/styling/lib/colorSpace.tsx`:
- Around line 83-98: The interactive color token resolution in colorSpace.tsx no
longer matches the spec expectations because it now uses the generic surface
overlay mixin token instead of surfaceType-specific suffixes. Update the
implementation around the InteractiveStateProps-driven color mix logic to either
restore distinct default/inverse surface token handling or align the tests and
docs with the new generic overlay-mixin behavior. Also update
modules/styling/spec/colorSpace.spec.ts to remove surfaceType usage and assert
the new resolved token strings consistently.
🪄 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: 13bafb38-215e-4b5a-a075-c0c9ffab587f
📒 Files selected for processing (1)
modules/styling/lib/colorSpace.tsx
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/styling/lib/colorSpace.tsx (1)
83-98: 🎯 Functional Correctness | 🔴 CriticalCorrect implementation and tests must be updated to address mismatch.
The
InteractiveStatePropsinterface correctly removedsurfaceType, but the function implementation inmodules/styling/lib/colorSpace.tsxnow resolves tokens tosystem.legacy.color[colorType].overlay.mixin(a single generic color) rather than the specificsurfaceType-suffixed tokens expected by tests.This results in:
- Token Regression: The CSS generated no longer differentiates between
defaultandinversesurface variants, collapsing them to--cnvs-sys-color-surface-overlay-mixin.- Failing Tests:
modules/styling/spec/colorSpace.spec.ts(lines 33-39, 58-64) explicitly passes the undefinedsurfaceTypeproperty and expects the old token structure (e.g.,--cnvs-sys-color-surface-overlay-hover-default).- Type Safety: Callers passing
surfaceTypewill encounter TypeScript errors ifInteractiveStatePropsremains the interface.The implementation logic should either be updated to support the specific token suffixes if distinct behavior is required, or the tests/documentation must be updated to remove
surfaceTypearguments and update expected output strings to match the genericoverlay-mixintoken being used.Comparison
Current Implementation Output:
color-mix(in srgb, var(--cnvs-sys-color-brand-accent-primary, var(--cnvs-brand-primary-base)) , var(--cnvs-sys-color-surface-overlay-mixin, var(black)) calc(var(--cnvs-sys-opacity-surface-hover, 0) * 100%))Test File Expectation (lines 38, 63):
// Expects distinct hover vs pressed suffixes and default suffix var(--cnvs-sys-color-surface-overlay-hover-default, black)🤖 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/styling/lib/colorSpace.tsx` around lines 83 - 98, The interactive color token resolution in colorSpace.tsx no longer matches the spec expectations because it now uses the generic surface overlay mixin token instead of surfaceType-specific suffixes. Update the implementation around the InteractiveStateProps-driven color mix logic to either restore distinct default/inverse surface token handling or align the tests and docs with the new generic overlay-mixin behavior. Also update modules/styling/spec/colorSpace.spec.ts to remove surfaceType usage and assert the new resolved token strings consistently.
🤖 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/styling/lib/colorSpace.tsx`:
- Around line 113-120: The hover and pressed state color helpers are both using
the wrong overlay token path, so update the state-specific mixin selection in
the interactive color helpers in colorSpace.tsx: have hover resolve from
system.legacy.color[colorType].overlay.hover and pressed resolve from
system.legacy.color[colorType].overlay.pressed instead of overlay.mixin, keeping
the existing darken call structure intact so the generated CSS variables match
the spec and the hover/pressed token names remain distinct.
---
Outside diff comments:
In `@modules/styling/lib/colorSpace.tsx`:
- Around line 83-98: The interactive color token resolution in colorSpace.tsx no
longer matches the spec expectations because it now uses the generic surface
overlay mixin token instead of surfaceType-specific suffixes. Update the
implementation around the InteractiveStateProps-driven color mix logic to either
restore distinct default/inverse surface token handling or align the tests and
docs with the new generic overlay-mixin behavior. Also update
modules/styling/spec/colorSpace.spec.ts to remove surfaceType usage and assert
the new resolved token strings consistently.
🪄 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: 13bafb38-215e-4b5a-a075-c0c9ffab587f
📒 Files selected for processing (1)
modules/styling/lib/colorSpace.tsx
🛑 Comments failed to post (1)
modules/styling/lib/colorSpace.tsx (1)
113-120: 🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect the legacy token shape used by the new mapping rg -nP "legacy\b" modules/styling/lib/ -g '!*.spec.*' -C2 fd -t f 'tokens' modules/styling/lib --exec rg -nP 'overlay|mixin|legacy' {} \; # Show current spec expectations to compare against the implementation fd 'colorSpace.spec.ts' --exec cat -n {}Repository: Workday/canvas-kit
Length of output: 4441
Correct
overlaytoken mapping for hover and pressed statesThe current implementation uses
system.legacy.color[colorType].overlay.mixinfor both thehoverandpressedfunctions. This is incorrect and conflicts with the spec:
- Token Mismatch: The
hovermethod resolves to--cnvs-sys-color-${colorType}-overlay-mixin, but the spec (e.g., modules/styling/spec/colorSpace.spec.ts) asserts the output must resolve to--cnvs-sys-color-${colorType}-overlay-hover. Similarly,pressedexpects...-overlay-pressed.- Semantics: Token definitions suggest
overlay.mixinis intended for a base or specific pressed state, not for hover.Update the
mixinColorlogic to select the state-specific token path (e.g.,overlay.hovervsoverlay.pressed) to ensure the generated CSS variables match the system design tokens and passing specs.Details
Current (Incorrect):
// hover.tsx mixinColor: system.legacy.color[colorType].overlay.mixin, // pressed.tsx mixinColor: system.legacy.color[colorType].overlay.mixin,Expected (Per Spec):
- hover: resolves to
var(--cnvs-sys-color-accent-overlay-hover, black)- pressed: resolves to
var(--cnvs-sys-color-accent-overlay-pressed, black)🤖 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/styling/lib/colorSpace.tsx` around lines 113 - 120, The hover and pressed state color helpers are both using the wrong overlay token path, so update the state-specific mixin selection in the interactive color helpers in colorSpace.tsx: have hover resolve from system.legacy.color[colorType].overlay.hover and pressed resolve from system.legacy.color[colorType].overlay.pressed instead of overlay.mixin, keeping the existing darken call structure intact so the generated CSS variables match the spec and the hover/pressed token names remain distinct.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/styling/spec/colorSpace.spec.ts (1)
49-51: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPressed accent expectation uses the wrong overlay token.
colorSpace.pressed()currently readssystem.legacy.color[colorType].overlay.mixin, so this test should assert--cnvs-sys-color-${colorType}-overlay-mixin(notoverlay-pressed).Proposed fix
- `color-mix(in srgb, var(${color}, var(${fallback})) , var(--cnvs-sys-color-${colorType}-overlay-pressed, black) calc(var(--cnvs-sys-opacity-${colorType}-pressed, 0) * 100%))` + `color-mix(in srgb, var(${color}, var(${fallback})) , var(--cnvs-sys-color-${colorType}-overlay-mixin, black) calc(var(--cnvs-sys-opacity-${colorType}-pressed, 0) * 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/styling/spec/colorSpace.spec.ts` around lines 49 - 51, The pressed accent expectation is using the wrong overlay token. Update the assertion in colorSpace.spec.ts for colorSpace.pressed() to match the implementation in system.legacy.color[colorType].overlay.mixin, and change the expected variable reference from overlay-pressed to --cnvs-sys-color-${colorType}-overlay-mixin so the test aligns with the actual token used by colorSpace.pressed().
🤖 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.
Outside diff comments:
In `@modules/styling/spec/colorSpace.spec.ts`:
- Around line 49-51: The pressed accent expectation is using the wrong overlay
token. Update the assertion in colorSpace.spec.ts for colorSpace.pressed() to
match the implementation in system.legacy.color[colorType].overlay.mixin, and
change the expected variable reference from overlay-pressed to
--cnvs-sys-color-${colorType}-overlay-mixin so the test aligns with the actual
token used by colorSpace.pressed().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 71fc4a20-1abf-46ba-a483-80d2622b6b4d
📒 Files selected for processing (1)
modules/styling/spec/colorSpace.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/styling/spec/colorSpace.spec.ts (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse surface-oriented base tokens in the “surface” test cases.
Both surface cases still pass accent base/fallback tokens, which makes the test intent less clear and less representative of a true surface path.
Also applies to: 53-56
🤖 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/styling/spec/colorSpace.spec.ts` around lines 29 - 32, The surface-focused colorSpace.spec tests are still using accent base/fallback tokens, so update the surface cases to use surface-oriented token names instead. In the relevant test blocks in colorSpace.spec.ts, adjust the values in the color-mix setup so the identifiers used by the test clearly reflect the surface path while keeping the existing behavior and assertions intact. Use the test case definitions around the surface “hover state” and the other surface case to rename the token variables consistently.
🤖 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.
Nitpick comments:
In `@modules/styling/spec/colorSpace.spec.ts`:
- Around line 29-32: The surface-focused colorSpace.spec tests are still using
accent base/fallback tokens, so update the surface cases to use surface-oriented
token names instead. In the relevant test blocks in colorSpace.spec.ts, adjust
the values in the color-mix setup so the identifiers used by the test clearly
reflect the surface path while keeping the existing behavior and assertions
intact. Use the test case definitions around the surface “hover state” and the
other surface case to rename the token variables consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 423512aa-b81e-4f9f-8d02-c0d76ae795b5
📒 Files selected for processing (1)
modules/styling/spec/colorSpace.spec.ts
There was a problem hiding this comment.
Pull request overview
Updates button-related styling/utilities to align interaction color mixing and button sizing with the Sana Canvas visual direction, and expands Storybook coverage for new/updated button states.
Changes:
- Switched
colorSpace.hover/pressedto usesystem.legacyoverlay/opacity tokens and expanded corresponding unit tests. - Updated button sizing/padding rules (including extra-small icon spacing) and refined state styling for Primary/Secondary/Tertiary/Delete buttons.
- Added Storybook examples/docs and visual-testing permutations for
DeleteButton’sinversevariant; adjustedSegmentedControl.Itemsizing.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/styling/spec/colorSpace.spec.ts | Updates/expands hover/pressed color-mix expectations and adds surface-vs-accent coverage. |
| modules/styling/lib/colorSpace.tsx | Routes hover/pressed mixing through system.legacy overlay/opacity token paths. |
| modules/react/segmented-control/lib/SegmentedControlItem.tsx | Tweaks item sizing via minWidth: 'min-content'. |
| modules/react/button/stories/visual-testing/DeleteButton.stories.tsx | Adds variant permutations including inverse for visual testing. |
| modules/react/button/stories/button/examples/DeleteInverse.tsx | Adds a new Storybook example showcasing DeleteButton inverse usage. |
| modules/react/button/stories/button/Button.stories.tsx | Registers the new DeleteInverse example as a story. |
| modules/react/button/stories/button/Button.mdx | Documents the DeleteButton inverse variant with an example snippet. |
| modules/react/button/lib/TertiaryButton.tsx | Refactors state styling and focus treatment; adjusts how iconPosition is passed to the stencil. |
| modules/react/button/lib/SecondaryButton.tsx | Refines border/label tokens and removes background overlay on hover/active. |
| modules/react/button/lib/PrimaryButton.tsx | Updates hover/pressed styling to use colorSpace.hover/pressed. |
| modules/react/button/lib/DeleteButton.tsx | Introduces variant="inverse" styling and threads variant into the stencil call. |
| modules/react/button/lib/BaseButton.tsx | Updates size padding/minWidth rules and adds extra-small icon-position padding using calc.add. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| iconPosition={iconPosition} | ||
| grow={grow} | ||
| cs={[ | ||
| tertiaryButtonStencil({ | ||
| size, |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md`:
- Around line 58-60: The codemod examples have malformed Markdown fences because
the `npx` sentence runs directly into the opening code fence, so update the
upgrade guide text to put a newline before each ` ```sh ` block. Fix both the
main codemod example and the later reference section by keeping the explanatory
sentence separate and ensuring the fenced snippet is wrapped cleanly around the
`npx `@workday/canvas-kit-codemod` v16 [path]` command.
- Around line 7-10: Remove the unfinished placeholder text from the upgrade
guide before publishing. Update the sections containing the “Why You Should
Upgrade” copy and the other referenced placeholder block so they contain real
upgrade guidance instead of “Soon to be...” or “[Content will be added]”. Use
the existing section titles in the markdown file to locate and replace the
template content with final prose.
- Around line 159-218: The Codemod Reference section still says there are no v16
transformations, which is stale and conflicts with the upgrade guidance. Update
the content in the Codemod Reference/Running a Codemod area to point readers to
the actual v16 migration codemod for the breaking button API changes, and remove
the unsupported “No codemod transformations available for v16” note. Keep the
instructions aligned with the existing `canvasKitMajorVersionNumber` codemod
examples so users know how to run the correct v16 migration path.
🪄 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: c2a9608a-f50d-46e2-92e1-13ba7444ccc4
📒 Files selected for processing (4)
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/mdx/16.0-UPGRADE-GUIDE.mdxmodules/react/button/lib/BaseButton.tsxmodules/react/button/stories/button/examples/DeleteInverse.tsx
✅ Files skipped from review due to trivial changes (2)
- modules/react/button/stories/button/examples/DeleteInverse.tsx
- modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/react/button/lib/BaseButton.tsx
| ## Why You Should Upgrade | ||
|
|
||
| Soon to be... | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the placeholder copy before publishing.
Soon to be... and [Content will be added] read like unfinished template text in a public upgrade guide.
Also applies to: 83-86
🤖 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/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md` around lines 7 - 10,
Remove the unfinished placeholder text from the upgrade guide before publishing.
Update the sections containing the “Why You Should Upgrade” copy and the other
referenced placeholder block so they contain real upgrade guidance instead of
“Soon to be...” or “[Content will be added]”. Use the existing section titles in
the markdown file to locate and replace the template content with final prose.
There was a problem hiding this comment.
@RayRedGoose Understood — okay to leave it as-is for this PR. If you'd like, I can open a small follow-up issue to replace the placeholder upgrade-guide copy before release.
(◕x◕)
| ## Codemod Reference | ||
|
|
||
| # What is a Codemod? | ||
|
|
||
| A codemod is a script that makes programmatic transformations on your codebase by traversing the | ||
| [AST](https://www.codeshiftcommunity.com/docs/understanding-asts), identifying patterns, and making | ||
| prescribed changes. This greatly decreases opportunities for error and reduces the number of manual | ||
| updates, which allows you to focus on changes that need your attention. **We highly recommend you | ||
| use the codemod for these reasons.** | ||
|
|
||
| If you're new to running codemods or if it's been a minute since you've used one, there are a few | ||
| things you'll want to keep in mind. | ||
|
|
||
| - Our codemods are meant to be run sequentially. For example, if you're using v8 of Canvas Kit, | ||
| you'll need to run the v9 codemod before you run v10 and so on. | ||
| - The codemod will update your code to be compatible with the specified version, but it will **not** | ||
| remove outdated dependencies or upgrade dependencies to the latest version. You'll need to upgrade | ||
| dependencies on your own. | ||
| - We recommend upgrading dependencies before running the codemod. | ||
| - Always review your `package.json` files to make sure your dependency versions look correct. | ||
| - The codemod will not handle every breaking change in this upgrade. You will likely need to make some manual | ||
| changes to be compatible. Use our Upgrade Guide as a checklist. | ||
| - Codemods are not bulletproof. | ||
| - Conduct a thorough PR and QA review of all changes to ensure no regressions were introduced. | ||
| - As a safety precaution, we recommend committing the changes from the codemod as a single | ||
| isolated commit (separate from other changes) so you can roll back more easily if necessary. | ||
|
|
||
| We're here to help! Automatic changes to your codebase can feel scary. You can always reach out to | ||
| our team. We'd be very happy to walk you through the process to set you up for success. | ||
|
|
||
| ## Running a Codemod | ||
|
|
||
| ### Instructions | ||
|
|
||
| The easiest way to run our codemod is to use `npx` in your terminal.```sh | ||
| npx @workday/canvas-kit-codemod v${canvasKitMajorVersionNumber} [path] | ||
| ``` | ||
|
|
||
| Be sure to provide specific directories that need to be updated via the `[path]` argument. This | ||
| decreases the amount of AST the codemod needs to traverse and reduces the chances of the script | ||
| having an error. For example, if your source code lives in `src/`, use `src/` as your `[path]`. Or, | ||
| if you have a monorepo with three packages using Canvas Kit, provide those specific packages as your | ||
| `[path]`. | ||
|
|
||
| Alternatively, if you're unable to run the codemod successfully using `npx`, you can install the | ||
| codemod package as a dev dependency, run it with `yarn`, and then remove the package after you're | ||
| finished. | ||
|
|
||
| ```sh | ||
| yarn add @workday/canvas-kit-codemod --dev | ||
| yarn canvas-kit-codemod v${canvasKitMajorVersionNumber} [path] | ||
| yarn remove @workday/canvas-kit-codemod | ||
| ``` | ||
|
|
||
| > **Note**: The codemod only works on `.js`, `.jsx`, `.ts`, and `.tsx` files. You'll need to | ||
| > manually edit other file types (`.json`, `.mdx`, `.md`, etc.). You may need to run your linter | ||
| > after executing the codemod, as its resulting formatting (spacing, quotes, etc.) may not match | ||
| > your project conventions. | ||
|
|
||
| No codemod transformations available for v16. No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace the stale v16 codemod note.
The reference section says there are no codemod transformations for v16, which contradicts the earlier codemod guidance and leaves readers without the migration path for the breaking button API changes.
🧰 Tools
🪛 LanguageTool
[style] ~179-~179: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...change in this upgrade. You will likely need to make some manual changes to be compat...
(REP_NEED_TO_VB)
[style] ~187-~187: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...n always reach out to our team. We'd be very happy to walk you through the process to set ...
(EN_WEAK_ADJECTIVE)
🤖 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/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md` around lines 159 -
218, The Codemod Reference section still says there are no v16 transformations,
which is stale and conflicts with the upgrade guidance. Update the content in
the Codemod Reference/Running a Codemod area to point readers to the actual v16
migration codemod for the breaking button API changes, and remove the
unsupported “No codemod transformations available for v16” note. Keep the
instructions aligned with the existing `canvasKitMajorVersionNumber` codemod
examples so users know how to run the correct v16 migration path.
| // Hover Styles | ||
| '&:hover, &.hover': { | ||
| [buttonStencil.vars.background]: colorSpace.darken({ | ||
| [buttonStencil.vars.background]: colorSpace.hover({ |
There was a problem hiding this comment.
I don't see any color change when I hover this button in Storybook. Are we missing some value in this util?
There was a problem hiding this comment.
In local, I'm seeing the interactive states.
There was a problem hiding this comment.
Why does it not work for me 🙃 Checked in a different browsers too. What version of tokens-web are you using locally?
There was a problem hiding this comment.
"@workday/canvas-tokens-web": "5.0.0-alpha.2" is what I'm currently using.
| mixinColor: system.legacy.color.surface.overlay.mixin, | ||
| mixinValue: system.legacy.opacity.surface.hover, | ||
| }), | ||
| [buttonStencil.vars.border]: system.legacy.color.border.inverse.default, |
There was a problem hiding this comment.
I'm not seeing a hover state for inverse SecondaryButton either:
There was a problem hiding this comment.
Oh I know why, because we removed the data-theme from tokens which separated it nicely for inverse variants.
There was a problem hiding this comment.
I've only seen the PrimaryButton work as it should.
| // Hover Styles | ||
| '&:hover, &.hover': { | ||
| [buttonStencil.vars.background]: colorSpace.darken({ | ||
| [buttonStencil.vars.background]: colorSpace.hover({ |
There was a problem hiding this comment.
Related to PrimaryButton, wondering if the ThemeOverrides story content is still fully accurate and worth keeping. I noticed the "Override Primary Color Via Canvas Provider" story renders PrimaryButtons that also don't show a hover color.
There was a problem hiding this comment.
Yeah, this is an issue with removing the data-theme from tokens.
sheelah
left a comment
sheelah
left a comment
There was a problem hiding this comment.
Thanks for tackling this tricky one! Left a few comments inline.
| { | ||
| variant: [ | ||
| {value: undefined, label: ''}, | ||
| {value: 'inverse', label: 'Inverse'}, |
There was a problem hiding this comment.
Do we want to wrap the inverse variant in a container with a dark background, for consistency with the other button components' inverse variant visual tests?
There was a problem hiding this comment.
Yeah I can update this.
There was a problem hiding this comment.
I did but, I'm not sure where this variant should exist?
I feel like this is now an accessibility issue with a dark background.
RayRedGoose
left a comment
RayRedGoose
left a comment
There was a problem hiding this comment.
Looking good. Left some small notes, but it's a good candidate to get merge once a11y review is done 👍
| ## Why You Should Upgrade | ||
|
|
||
| Soon to be... | ||
|
|
Summary
Fixes: #3976
Release Category
Components
Release Note
This updates our Button styling to introduce Sana Canvas.
BREAKING CHANGES
Visual Changes
Where Should the Reviewer Start?
/modules/react/buttonSummary by CodeRabbit
Summary by CodeRabbit
New Features
DeleteButton, including Storybook/MDX examples and visual-testing state permutations.DeleteButtoninverse variant.Style
colorSpace.hover()/colorSpace.pressed()to derive values from legacy tokens.Bug Fixes
SegmentedControl.Itemlayout by honoring content min-width (min-content).Tests
colorSpacehover/pressed coverage forcolorType(accentvssurface).