feat: Add KBD component to labs#4001
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a new ChangesKBD Compound Component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Workday/canvas-kit
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
issue3990-kbd-component
|
| Run status |
|
| Run duration | 02m 28s |
| Commit |
|
| Committer | Raisa Primerova |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
17
|
|
|
0
|
|
|
809
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.69%
|
|
|---|---|
|
|
1523
|
|
|
371
|
Accessibility
99.41%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
68
|
There was a problem hiding this comment.
Pull request overview
Introduces a new Labs KBD compound component (@workday/canvas-kit-labs-react/kbd) for rendering keyboard shortcuts with Storybook documentation, visual testing coverage, and an SSR smoke test.
Changes:
- Added
KBDandKBD.Itemimplementation usingcreateComponent+createStencil - Added Storybook stories/MDX docs and Chromatic visual-testing states
- Added an SSR Jest spec and exported
kbdfrom the Labs package entrypoint
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/labs-react/kbd/stories/visual-testing/testing.stories.tsx | Adds Chromatic visual regression coverage across variants/sizes |
| modules/labs-react/kbd/stories/tsconfig.json | Storybook TS config for the new KBD stories folder |
| modules/labs-react/kbd/stories/KBD.stories.tsx | Storybook entry wiring examples + MDX docs |
| modules/labs-react/kbd/stories/KBD.mdx | MDX documentation for KBD usage and API |
| modules/labs-react/kbd/stories/examples/Variant.tsx | Variant example for docs/stories |
| modules/labs-react/kbd/stories/examples/Size.tsx | Size example for docs/stories |
| modules/labs-react/kbd/stories/examples/RTL.tsx | RTL usage example for docs/stories |
| modules/labs-react/kbd/stories/examples/Basic.tsx | Basic usage example for docs/stories |
| modules/labs-react/kbd/spec/tsconfig.json | Jest/spec TS config for the new component |
| modules/labs-react/kbd/spec/SSR.spec.tsx | SSR smoke test for the new component |
| modules/labs-react/kbd/README.md | Component README for Labs package documentation |
| modules/labs-react/kbd/LICENSE | License file for the new module directory |
| modules/labs-react/kbd/lib/KBDItem.tsx | Implements KBD.Item subcomponent |
| modules/labs-react/kbd/lib/KBD.tsx | Implements KBD container + stencil |
| modules/labs-react/kbd/index.ts | Public exports for the new module |
| modules/labs-react/index.ts | Re-exports kbd from the Labs package root |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/labs-react/kbd/README.md (1)
5-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: Improve sentence flow and clarity.
Line 5 ends with "currently in prerelease." followed by line 7 "A compound component...". The flow is awkward. Consider combining the sentences:
- Option 1: "...currently in prerelease. This is a compound component..."
- Option 2: "...currently in prerelease: a compound component..."
- Option 3: "...currently in prerelease, a compound component..."
🤖 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/labs-react/kbd/README.md` around lines 5 - 7, The README.md file has awkward sentence flow between the prerelease status statement (ending with "currently in prerelease.") and the component description (starting with "A compound component..."). Combine these two sentences for better readability by either adding a transitional word like "This is", using a colon, or using a comma to connect the prerelease status with the compound component description in a single coherent statement.
🤖 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/labs-react/kbd/README.md`:
- Around line 5-7: The README.md file has awkward sentence flow between the
prerelease status statement (ending with "currently in prerelease.") and the
component description (starting with "A compound component..."). Combine these
two sentences for better readability by either adding a transitional word like
"This is", using a colon, or using a comma to connect the prerelease status with
the compound component description in a single coherent statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a22f1cf-ff8c-4067-ad99-bbaa388b4b1e
📒 Files selected for processing (16)
modules/labs-react/index.tsmodules/labs-react/kbd/LICENSEmodules/labs-react/kbd/README.mdmodules/labs-react/kbd/index.tsmodules/labs-react/kbd/lib/KBD.tsxmodules/labs-react/kbd/lib/KBDItem.tsxmodules/labs-react/kbd/spec/SSR.spec.tsxmodules/labs-react/kbd/spec/tsconfig.jsonmodules/labs-react/kbd/stories/KBD.mdxmodules/labs-react/kbd/stories/KBD.stories.tsxmodules/labs-react/kbd/stories/examples/Basic.tsxmodules/labs-react/kbd/stories/examples/RTL.tsxmodules/labs-react/kbd/stories/examples/Size.tsxmodules/labs-react/kbd/stories/examples/Variant.tsxmodules/labs-react/kbd/stories/tsconfig.jsonmodules/labs-react/kbd/stories/visual-testing/testing.stories.tsx
|
@Workday/canvas-engineering Do we want to support basic model with items? Now import {KBD} from '@workday/canvas-kit-labs-react';
<KBD>
<KBD.Item>⌘</KBD.Item>
<KBD.Item>C</KBD.Item>
</KBD>;and if there is a complex shortcut it could look like: import {KBD} from '@workday/canvas-kit-labs-react';
<KBD>
{[...].map(item => <KBD.Item>{item}</KBD.Item>)}
</KBD>;By adding model and items we can have a render function for multiple icons, it could have the next syntax: import {KBD} from '@workday/canvas-kit-labs-react';
<KBD items={[...]}>
{item => <KBD.Item>{item}</KBD.Item>}
</KBD>;I don't see there is a big different between these approaches, so decided to keep it simple for now. So I would like to hear your opinions on that. |
sheelah
left a comment
There was a problem hiding this comment.
Looking good. Left a few minor comments inline.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/labs-react/kbd/stories/KBD.mdx (1)
74-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument
KBD.Itemin the API section.The examples make
KBD.Itemthe primary public surface, but the API section only documentsKBDanduseKBDModel. Adding the item here will make the compound API discoverable without relying on the examples.♻️ Proposed change
## Component API <SymbolDoc name="KBD" hideDescription /> +<SymbolDoc name="KBD.Item" hideDescription /> ## Model🤖 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/labs-react/kbd/stories/KBD.mdx` around lines 74 - 94, The API section for KBD is missing the compound item component, so add documentation for KBD.Item alongside KBD and useKBDModel in the KBD.mdx story. Update the Component API section to include a SymbolDoc entry for KBD.Item so the public compound surface is discoverable without relying on the usage example.
🤖 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/labs-react/kbd/lib/KBD.tsx`:
- Around line 81-101: The dynamic items path in KBD may break for plain-string
`items` because `useListRenderItems(model, children)` relies on the collection’s
default item identity, which can resolve to undefined for primitive strings.
Inspect `KBD` and `useKBDModel` together with the underlying
`useListModel`/collection `getId` behavior, and update the dynamic rendering
path so string items get stable unique keys/ids before render while leaving the
static `KBD.Item` children flow unchanged.
---
Nitpick comments:
In `@modules/labs-react/kbd/stories/KBD.mdx`:
- Around line 74-94: The API section for KBD is missing the compound item
component, so add documentation for KBD.Item alongside KBD and useKBDModel in
the KBD.mdx story. Update the Component API section to include a SymbolDoc entry
for KBD.Item so the public compound surface is discoverable without relying on
the usage example.
🪄 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: 0011f9f8-f852-47ca-92e1-be5844a8dd82
📒 Files selected for processing (10)
modules/labs-react/kbd/index.tsmodules/labs-react/kbd/lib/KBD.tsxmodules/labs-react/kbd/lib/KBDItem.tsxmodules/labs-react/kbd/lib/useKBDModel.tsxmodules/labs-react/kbd/stories/KBD.mdxmodules/labs-react/kbd/stories/KBD.stories.tsxmodules/labs-react/kbd/stories/examples/Basic.tsxmodules/labs-react/kbd/stories/examples/Items.tsxmodules/labs-react/kbd/stories/examples/RTL.tsxmodules/labs-react/kbd/stories/visual-testing/testing.stories.tsx
✅ Files skipped from review due to trivial changes (3)
- modules/labs-react/kbd/stories/examples/Items.tsx
- modules/labs-react/kbd/index.ts
- modules/labs-react/kbd/stories/KBD.stories.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/labs-react/kbd/stories/examples/RTL.tsx
- modules/labs-react/kbd/stories/visual-testing/testing.stories.tsx
williamjstanton
left a comment
There was a problem hiding this comment.
Looks good to me, and fairly straightforward. I think there's a couple things we can improve in the docs noted in comments above. Thanks!
|
@williamjstanton added accessibility section and examples 👍 |
bcee319 to
199f6fb
Compare
Summary
Fixes: #3990
Adds a new KBD component to the Canvas Labs package for rendering keyboard shortcuts.
Release Category
Components
Release Note
A new
KBDcomponent has been added to the Canvas Labs (@workday/canvas-kit-labs-react) package for rendering keyboard shortcuts.BREAKING CHANGES
A new
KBDcomponent has been added to the Canvas Labs (@workday/canvas-kit-labs-react) package for rendering keyboard shortcuts.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