feat: Add corner-shape for components with border-radius#3968
Open
sheelah wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces a ChangesCorner Shape System Implementation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
add-corner-shape
|
| Run status |
|
| Run duration | 02m 26s |
| 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.58%
|
|
|---|---|
|
|
1534
|
|
|
371
|
Accessibility
99.41%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
68
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
modules/styling-transform/spec/handleWithCornerShape.spec.ts (1)
5-25: ⚡ Quick winExpand test coverage to include edge cases.
The test suite only covers the happy path with a string literal argument. Consider adding tests for:
- Missing arguments:
withCornerShape()- Multiple arguments:
withCornerShape('24px', 'extra')- Numeric arguments:
withCornerShape(24)- Non-static values (variables, expressions)
withCornerShapenot imported from@workday/canvas-kit-stylingThese edge cases will help catch issues like the missing argument validation flagged in
handleWithCornerShape.ts.🤖 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-transform/spec/handleWithCornerShape.spec.ts` around lines 5 - 25, Add additional unit tests to handle edge cases for the withCornerShape transform: add tests in handleWithCornerShape.spec.ts that call createProgramFromSource + transform with objectTransforms: [handleWithCornerShape] for (1) missing argument case withCornerShape() expecting no radius/corner-shape output or a handled error, (2) multiple arguments withCornerShape('24px', 'extra') ensuring only the first is used or it's rejected, (3) numeric argument withCornerShape(24) verifying numeric-to-string handling, (4) non-static/variable/expression arguments (e.g., const r='24px'; withCornerShape(r)) asserting it is not transformed, and (5) calls to withCornerShape from a different import (not `@workday/canvas-kit-styling`) to ensure the transformer ignores it; reference the existing test that uses handleWithCornerShape and withCornerShape to mirror setup and assertions (expecting border-radius and corner-shape presence or absence) for each case.modules/styling-transform/testing.ts (1)
2-2: Consider removing (or justifying) theparseObjectToStaticValuere-export from the testing barrel
modules/styling-transform/testing.tsre-exportsparseObjectToStaticValue, but no specs in this repo import it from@workday/canvas-kit-styling-transform/testing—the dedicated spec imports it directly frommodules/styling-transform/lib/utils/parseObjectToStaticValue. If this re-export isn’t required for consumers/other packages, it can likely be dropped to reduce the testing entrypoint surface area.export {parseObjectToStaticValue} from './lib/utils/parseObjectToStaticValue';🤖 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-transform/testing.ts` at line 2, The testing barrel currently re-exports parseObjectToStaticValue which appears unused by this package's specs—remove the line exporting parseObjectToStaticValue from modules/styling-transform/testing.ts to shrink the public testing surface; before deleting, run a repo-wide search for imports of parseObjectToStaticValue from `@workday/canvas-kit-styling-transform/testing` and, if any are found, update those imports to the original path (modules/styling-transform/lib/utils/parseObjectToStaticValue) or document/justify keeping the re-export.
🤖 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-transform/lib/utils/handleWithCornerShape.ts`:
- Around line 9-20: The transform handleWithCornerShape currently reads
node.arguments[0] without validating arguments; update the createObjectTransform
callback so it first checks ts.isCallExpression(node) && node.arguments &&
node.arguments.length === 1 before calling parseNodeToStaticValue, and if the
length !== 1 then early-return undefined (or emit a diagnostic via the provided
context) to avoid a crash for zero args and to surface/report extra args rather
than silently ignoring them; keep the existing checks for
isIdentifier/node.expression text and isImportedFromStyling and still return the
object with borderRadius: parseNodeToStaticValue(node.arguments[0], context) and
cornerShape: CORNER_SHAPE when validation passes.
---
Nitpick comments:
In `@modules/styling-transform/spec/handleWithCornerShape.spec.ts`:
- Around line 5-25: Add additional unit tests to handle edge cases for the
withCornerShape transform: add tests in handleWithCornerShape.spec.ts that call
createProgramFromSource + transform with objectTransforms:
[handleWithCornerShape] for (1) missing argument case withCornerShape()
expecting no radius/corner-shape output or a handled error, (2) multiple
arguments withCornerShape('24px', 'extra') ensuring only the first is used or
it's rejected, (3) numeric argument withCornerShape(24) verifying
numeric-to-string handling, (4) non-static/variable/expression arguments (e.g.,
const r='24px'; withCornerShape(r)) asserting it is not transformed, and (5)
calls to withCornerShape from a different import (not
`@workday/canvas-kit-styling`) to ensure the transformer ignores it; reference the
existing test that uses handleWithCornerShape and withCornerShape to mirror
setup and assertions (expecting border-radius and corner-shape presence or
absence) for each case.
In `@modules/styling-transform/testing.ts`:
- Line 2: The testing barrel currently re-exports parseObjectToStaticValue which
appears unused by this package's specs—remove the line exporting
parseObjectToStaticValue from modules/styling-transform/testing.ts to shrink the
public testing surface; before deleting, run a repo-wide search for imports of
parseObjectToStaticValue from `@workday/canvas-kit-styling-transform/testing` and,
if any are found, update those imports to the original path
(modules/styling-transform/lib/utils/parseObjectToStaticValue) or
document/justify keeping the re-export.
🪄 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: 921a0284-95c1-4f0d-8886-1c0370d5d1a6
📒 Files selected for processing (47)
modules/preview-react/color-picker/lib/parts/SwatchBook.tsxmodules/preview-react/multi-select/lib/MultiSelectInput.tsxmodules/preview-react/radio/lib/RadioGroup.tsxmodules/preview-react/status-indicator/lib/StatusIndicator.tsxmodules/react/banner/lib/Banner.tsxmodules/react/button/lib/Hyperlink.tsxmodules/react/button/lib/ToolbarDropdownButton.tsxmodules/react/button/lib/ToolbarIconButton.tsxmodules/react/card/lib/Card.tsxmodules/react/checkbox/lib/CheckBackground.tsxmodules/react/checkbox/lib/CheckboxInput.tsxmodules/react/color-picker/lib/parts/ColorSwatch.tsxmodules/react/combobox/lib/ComboboxCard.tsxmodules/react/expandable/lib/ExpandableTarget.tsxmodules/react/form-field/lib/FormFieldGroupList.tsxmodules/react/information-highlight/lib/InformationHighlight.tsxmodules/react/layout/lib/utils/buildStyleFns.tsmodules/react/layout/spec/border.spec.tsmodules/react/menu/lib/MenuCard.tsxmodules/react/menu/lib/MenuItem.tsxmodules/react/menu/lib/MenuList.tsxmodules/react/modal/lib/ModalCard.tsxmodules/react/pill/lib/Pill.tsxmodules/react/pill/lib/PillCount.tsxmodules/react/pill/lib/PillIconButton.tsxmodules/react/popup/lib/PopupCard.tsxmodules/react/radio/lib/RadioGroup.tsxmodules/react/segmented-control/lib/SegmentedControlItem.tsxmodules/react/segmented-control/lib/SegmentedControlList.tsxmodules/react/skeleton/lib/parts/SkeletonHeader.tsxmodules/react/skeleton/lib/parts/SkeletonShape.tsxmodules/react/skeleton/lib/parts/SkeletonText.tsxmodules/react/status-indicator/lib/StatusIndicator.tsxmodules/react/table/lib/BaseTable.tsxmodules/react/tabs/lib/TabsItem.tsxmodules/react/text-input/lib/TextInput.tsxmodules/react/toast/lib/Toast.tsxmodules/react/tooltip/lib/TooltipContainer.tsxmodules/styling-transform/index.tsmodules/styling-transform/lib/utils/handleWithCornerShape.tsmodules/styling-transform/spec/createProgramFromSource.tsmodules/styling-transform/spec/handleWithCornerShape.spec.tsmodules/styling-transform/testing.tsmodules/styling/index.tsmodules/styling/lib/cornerShape.tsmodules/styling/spec/cornerShape.spec.tsstyling.config.ts
Comment on lines
+9
to
+20
| export const handleWithCornerShape = createObjectTransform((node, context) => { | ||
| if ( | ||
| ts.isCallExpression(node) && | ||
| ts.isIdentifier(node.expression) && | ||
| node.expression.text === 'withCornerShape' && | ||
| isImportedFromStyling(node.expression, context.checker) | ||
| ) { | ||
| return { | ||
| borderRadius: parseNodeToStaticValue(node.arguments[0], context), | ||
| cornerShape: CORNER_SHAPE, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add argument validation before accessing node.arguments[0].
The transform accesses node.arguments[0] without verifying that exactly one argument was provided. If withCornerShape() is called with no arguments, this will cause a compilation error. If called with multiple arguments, the extras will be silently ignored.
🛡️ Proposed fix to add argument validation
export const handleWithCornerShape = createObjectTransform((node, context) => {
if (
ts.isCallExpression(node) &&
ts.isIdentifier(node.expression) &&
node.expression.text === 'withCornerShape' &&
- isImportedFromStyling(node.expression, context.checker)
+ isImportedFromStyling(node.expression, context.checker) &&
+ node.arguments.length === 1
) {
return {
borderRadius: parseNodeToStaticValue(node.arguments[0], context),
cornerShape: CORNER_SHAPE,
};
}
return;
});🤖 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-transform/lib/utils/handleWithCornerShape.ts` around lines 9
- 20, The transform handleWithCornerShape currently reads node.arguments[0]
without validating arguments; update the createObjectTransform callback so it
first checks ts.isCallExpression(node) && node.arguments &&
node.arguments.length === 1 before calling parseNodeToStaticValue, and if the
length !== 1 then early-return undefined (or emit a diagnostic via the provided
context) to avoid a crash for zero args and to surface/report extra args rather
than silently ignoring them; keep the existing checks for
isIdentifier/node.expression text and isImportedFromStyling and still return the
object with borderRadius: parseNodeToStaticValue(node.arguments[0], context) and
cornerShape: CORNER_SHAPE when validation passes.
use CORNER_SHAPE inline, instead of a helper that couldn't be applied consistently due to different use cases of border-radius (shorthand vs longhand, etc).
sheelah
added
the
experimental
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Draft PR to test the visual changes from adding
corner-shapeon our elements that use border-radius. Circular elements have been left unchanged.Note: This feature is NOT yet supported across all browsers (including Safari and Firefox) so we're using progressive enhancement. Browsers that don't support this property will fall back to using border-radius. This property also does nothing without border-radius also declared.
The desired value to use directly after
border-radiusis declared will be:cornerShape: superellipse(1.1)Release Category
Components
Release Note
Optional release note message. Changelog and release summaries will contain a pull request title. This section will add additional notes under that title. This section is not a summary, but something extra to point out in release notes. An example might be calling out breaking changes in a labs component or minor visual changes that need visual regression updates. Remove this section if no additional release notes are required.
BREAKING CHANGES
Optional breaking changes message. If your PR includes breaking changes. It is extremely rare to put breaking changes outside a
prerelease/majorbranch. Anything in this section will show up in release notes. Remove this section if no breaking changes are present.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
Release Notes