feat: Add Sana Canvas Navigation component updates#4068
Conversation
Adds type=inline|standalone and a secondary variant option, so that color and underline are controlled independently.
|
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 Hyperlink APIs and migration tooling for v16, aligns Breadcrumbs and Pagination styling with new tokens, refreshes Storybook examples and visual states, and documents the navigation changes and codemod usage. ChangesSana navigation updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant v16Codemod
participant updateHyperlinkProps
participant JSXAST
v16Codemod->>updateHyperlinkProps: execute registered transform
updateHyperlinkProps->>JSXAST: inspect imports and JSX props
JSXAST-->>updateHyperlinkProps: return matching elements
updateHyperlinkProps->>JSXAST: rewrite variant and type props
JSXAST-->>v16Codemod: serialize transformed source
Possibly related PRs
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 |
![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
🧹 Nitpick comments (4)
modules/react/button/lib/Hyperlink.tsx (1)
35-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove extraneous space in the
paddingvalue.While it is valid CSS, removing the trailing space in the padding template literal improves code cleanliness.
♻️ Proposed refactor
- padding: `0 ${px2rem(2)} `, + padding: `0 ${px2rem(2)}`,🤖 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/Hyperlink.tsx` around lines 35 - 52, Update the padding declaration in the Hyperlink styles to remove the extraneous trailing space from its template literal while preserving the existing horizontal padding value.modules/react/button/stories/visual-testing/ExternalHyperlink.stories.tsx (1)
35-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the
typeprop in visual permutations.Consider adding the
typeprop to theComponentStatesTablepermutations so that thestandalonebehavior (no underline) ofExternalHyperlinkis visually tested, matching the test coverage just added toHyperlink.stories.tsx.♻️ Proposed refactor
rowProps={permutateProps({ variant: [ {label: 'Default', value: undefined}, {label: 'Inverse', value: 'inverse'}, {label: 'Secondary', value: 'secondary'}, ], + type: [ + {label: 'Inline', value: undefined}, + {label: 'Standalone', value: 'standalone'}, + ], })}🤖 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/stories/visual-testing/ExternalHyperlink.stories.tsx` at line 35, Update the ComponentStatesTable permutations in ExternalHyperlink.stories.tsx to include the ExternalHyperlink type values, including standalone, so its no-underline behavior is covered by visual testing consistently with Hyperlink.stories.tsx.modules/react/button/stories/button/examples/SecondaryHyperlink.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unused import.
ExternalHyperlinkis imported but never used in this example.♻️ Proposed fix
-import {ExternalHyperlink, Hyperlink} from '`@workday/canvas-kit-react/button`'; +import {Hyperlink} from '`@workday/canvas-kit-react/button`';🤖 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/stories/button/examples/SecondaryHyperlink.tsx` at line 1, Remove the unused ExternalHyperlink import from the SecondaryHyperlink example, while retaining the Hyperlink import used by the example.modules/react/button/stories/button/Hyperlink.mdx (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the phrasing.
As per static analysis hints, the phrase "outside of a" can be slightly redundant. Consider using "outside a" to be more concise.
♻️ Proposed fix
-Use `type="standalone"` when you need to place a `Hyperlink` outside of a paragraph or body text. +Use `type="standalone"` when you need to place a `Hyperlink` outside a paragraph or body text.🤖 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/stories/button/Hyperlink.mdx` at line 55, Update the Hyperlink usage guidance to replace “outside of a paragraph or body text” with the more concise “outside a paragraph or body text,” preserving the existing meaning and the type="standalone" instruction.Source: Linters/SAST tools
🤖 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/codemod/lib/v16/updateHyperlinkProps.ts`:
- Around line 28-37: Update ensureTypeStandalone and its existing type-attribute
detection so any JSX type attribute is recognized, regardless of value, then
overwrite that attribute in place with standalone instead of appending a
duplicate. Preserve the current behavior for elements without a type attribute
and add coverage for a Hyperlink with variant="standalone" and type="inline".
---
Nitpick comments:
In `@modules/react/button/lib/Hyperlink.tsx`:
- Around line 35-52: Update the padding declaration in the Hyperlink styles to
remove the extraneous trailing space from its template literal while preserving
the existing horizontal padding value.
In `@modules/react/button/stories/button/examples/SecondaryHyperlink.tsx`:
- Line 1: Remove the unused ExternalHyperlink import from the SecondaryHyperlink
example, while retaining the Hyperlink import used by the example.
In `@modules/react/button/stories/button/Hyperlink.mdx`:
- Line 55: Update the Hyperlink usage guidance to replace “outside of a
paragraph or body text” with the more concise “outside a paragraph or body
text,” preserving the existing meaning and the type="standalone" instruction.
In `@modules/react/button/stories/visual-testing/ExternalHyperlink.stories.tsx`:
- Line 35: Update the ComponentStatesTable permutations in
ExternalHyperlink.stories.tsx to include the ExternalHyperlink type values,
including standalone, so its no-underline behavior is covered by visual testing
consistently with Hyperlink.stories.tsx.
🪄 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: 16a5837f-747e-4b60-bf75-32c29bfe23de
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
modules/codemod/lib/v16/index.tsmodules/codemod/lib/v16/spec/updateHyperlinkProps.spec.tsmodules/codemod/lib/v16/updateHyperlinkProps.tsmodules/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/breadcrumbs/lib/BreadcrumbsCurrentItem.tsxmodules/react/breadcrumbs/lib/BreadcrumbsItem.tsxmodules/react/breadcrumbs/lib/BreadcrumbsLink.tsxmodules/react/breadcrumbs/lib/BreadcrumbsOverflowButton.tsxmodules/react/button/lib/ExternalHyperlink.tsxmodules/react/button/lib/Hyperlink.tsxmodules/react/button/stories/button/Hyperlink.mdxmodules/react/button/stories/button/Hyperlink.stories.tsxmodules/react/button/stories/button/examples/ExternalHyperlinkInverse.tsxmodules/react/button/stories/button/examples/HyperlinkInverse.tsxmodules/react/button/stories/button/examples/SecondaryHyperlink.tsxmodules/react/button/stories/button/examples/StandaloneHyperlink.tsxmodules/react/button/stories/visual-testing/ExternalHyperlink.stories.tsxmodules/react/button/stories/visual-testing/Hyperlink.stories.tsxmodules/react/package.jsonmodules/react/pagination/lib/Pagination/Controls.tsxmodules/react/pagination/lib/Pagination/GoTo/Form.tsxmodules/react/pagination/lib/Pagination/GoTo/Label.tsxmodules/react/pagination/lib/Pagination/GoTo/TextInput.tsxmodules/react/pagination/lib/Pagination/PageButton.tsxmodules/styling-transform/package.jsonmodules/styling/package.jsonpackage.json
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
sana-canvas-navigation
|
| Run status |
|
| Run duration | 02m 30s |
| 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.6%
|
|
|---|---|
|
|
1532
|
|
|
371
|
Accessibility
99.47%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
72
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx (1)
440-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary comma.
The comma before "and" is unnecessary when listing only two items ("
standaloneandstandaloneInverse").
modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx#L440-L441: Remove the comma after`standalone`.modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md#L434-L435: Remove the comma after`standalone`.🤖 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/mdx/16.0-UPGRADE-GUIDE.mdx` around lines 440 - 441, Remove the unnecessary comma after `standalone` in the compound-variants sentence in both modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx (lines 440-441) and modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md (lines 434-435), leaving the wording otherwise unchanged.
🤖 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`:
- Line 77: Insert an empty line before each fenced shell code block in
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md at lines 77 and 579,
ensuring the preceding prose ends on its own line and both ```sh fences render
as code blocks.
---
Nitpick comments:
In `@modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx`:
- Around line 440-441: Remove the unnecessary comma after `standalone` in the
compound-variants sentence in both modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
(lines 440-441) and modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md (lines
434-435), leaving the wording otherwise unchanged.
🪄 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: 0814d628-6e6e-408c-b02a-a40302431b74
📒 Files selected for processing (2)
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
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
🧹 Nitpick comments (1)
modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx (1)
440-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary comma.
The comma before "and" is unnecessary when listing only two items ("
standaloneandstandaloneInverse").
modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx#L440-L441: Remove the comma after`standalone`.modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md#L434-L435: Remove the comma after`standalone`.🤖 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/mdx/16.0-UPGRADE-GUIDE.mdx` around lines 440 - 441, Remove the unnecessary comma after `standalone` in the compound-variants sentence in both modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx (lines 440-441) and modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md (lines 434-435), leaving the wording otherwise unchanged.
🤖 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`:
- Line 77: Insert an empty line before each fenced shell code block in
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md at lines 77 and 579,
ensuring the preceding prose ends on its own line and both ```sh fences render
as code blocks.
---
Nitpick comments:
In `@modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx`:
- Around line 440-441: Remove the unnecessary comma after `standalone` in the
compound-variants sentence in both modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
(lines 440-441) and modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md (lines
434-435), leaving the wording otherwise unchanged.
🪄 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: 0814d628-6e6e-408c-b02a-a40302431b74
📒 Files selected for processing (2)
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
🛑 Comments failed to post (1)
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md (1)
77-77: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a newline before the code block.
Markdown requires an empty line before a code block (
```) to render it properly. Without it, the code block will not format as expected and will display inline.
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md#L77-L77: Add a line break before```sh.modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md#L579-L579: Add a line break before```sh.📍 Affects 1 file
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md#L77-L77(this comment)modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md#L579-L579🤖 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` at line 77, Insert an empty line before each fenced shell code block in modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md at lines 77 and 579, ensuring the preceding prose ends on its own line and both ```sh fences render as code blocks.
Update tokens package to latest beta [category:Dependencies] Co-authored-by: manuel.carrera <manuel.carrera@workday.com>
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/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md`:
- Around line 581-584: Normalize both codemod examples in the upgrade guide,
including the additionally affected block, by placing `npx` inside each `sh`
fenced code block with the command and replacing every four-backtick closing
fence with a standard three-backtick fence.
🪄 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: efdef4cd-af11-498b-b3d9-7446df323434
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
modules/codemod/lib/v16/spec/updateHyperlinkProps.spec.tsmodules/codemod/lib/v16/updateHyperlinkProps.tsmodules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/package.jsonmodules/labs-react/package.jsonmodules/preview-react/package.jsonmodules/react/button/lib/Hyperlink.tsxmodules/react/button/stories/button/Hyperlink.mdxmodules/react/button/stories/button/examples/SecondaryHyperlink.tsxmodules/react/checkbox/lib/CheckboxInput.tsxmodules/react/package.jsonmodules/styling-transform/package.jsonmodules/styling/package.jsonpackage.json
💤 Files with no reviewable changes (1)
- modules/react/checkbox/lib/CheckboxInput.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- modules/react/button/stories/button/examples/SecondaryHyperlink.tsx
- modules/codemod/lib/v16/updateHyperlinkProps.ts
- modules/codemod/lib/v16/spec/updateHyperlinkProps.spec.ts
- modules/react/button/lib/Hyperlink.tsx
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/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.md`:
- Around line 614-615: Replace the interpolated ${canvasKitMajorVersionNumber}
argument in the codemod command with the concrete v16 version, matching the
static guide’s existing examples.
🪄 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: 02f0f09c-b656-49f5-8360-61f4f45ebaa2
📒 Files selected for processing (2)
modules/docs/llm/upgrade-guides/16.0-UPGRADE-GUIDE.mdmodules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/docs/mdx/16.0-UPGRADE-GUIDE.mdx
| '&:active, &.active': { | ||
| color: system.legacy.color.fg.info.strong, | ||
| background: system.legacy.color.surface.alt.default, | ||
| color: system.sana.color.brand.fg.link.hover, |
There was a problem hiding this comment.
should this be a brandable token? same for above?
There was a problem hiding this comment.
@mannycarrera4 That's a good question. Still wrapping my head around our current token hierarchy (legacy vs non-legacy etc). At the end of the day, who decides what gets a brandable token?
There was a problem hiding this comment.
Left the color as-is for the Hyperlink states as there is no system.legacy.* or system.* equivalent, but I updated the focus ring color to be brandable.
system.legacy.color.brand.border.primary is not brandable but system.legacy.color.brand.focus.primary is according to CanvasProvider 's focusOutline theme, so I swapped that in.
There was a problem hiding this comment.
anything that has the brand word in it is brandable
There was a problem hiding this comment.
I just don't think that hyperlinks should be brandable if someone choose purple, i think a hyperlink should still be blue
ba72c7d to
0222b51
Compare
2 participants
Summary
Fixes: #3986.
Note: I filed #4070 for the Breadcrumbs overflow button rendering issues we're seeing in Storybook.
Release Category
Components
Release Note
This introduces a new
typevariant forHyperlinkandExternalHyperlink, defaulting to "inline". Set it to "standalone" to remove a link's underline.BREAKING CHANGES
The
HyperlinkandExternalHyperlinkstandaloneandstandaloneInversevariants have been removed, in favor of using a newtypeprop which is eitherinline(default) orstandalone. A codemod is available to migrate to the new component API.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
variant(color) andtype(underline).type-driven underline behavior, plus new Secondary hyperlink examples and visual-state coverage.