docs: Add corner shape and parts conventions to STYLE.md - #4130
Open
sheelah wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughSTYLE.md adds rules for ChangesSTYLE.md stencil guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 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 `@STYLE.md`:
- Line 159: Add the missing period after “etc.” in the parenthetical phrase so
the sentence ends with “etc.).”
- Around line 221-223: Update the prop-spread guidance to match handleCsProp’s
behavior: spread handleCsProp(elemProps, ...) before stencil.parts.x when the
stencil part identifier must take precedence, or revise the guarantee to
explicitly allow consumer data-part overrides. Preserve the documented ordering
for fixed props that must override handleCsProp.
🪄 Autofix
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: 41da1d37-d11a-4e09-a4a5-19e55d1a2f8f
📒 Files selected for processing (1)
STYLE.md
| [corner-shape](https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/corner-shape) | ||
| to components that use border-radius under certain conditions (see below), using a | ||
| `cornerShapeStencil`. This applies to any stencil with a border radius | ||
| (inputs, cards, menu items, buttons, etc). The one exception is a stencil that already extends |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the period after etc..
Line 159 should use etc.).
Proposed fix
- (inputs, cards, menu items, buttons, etc). The one exception is a stencil that already extends
+ (inputs, cards, menu items, buttons, etc.). The one exception is a stencil that already extends📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| (inputs, cards, menu items, buttons, etc). The one exception is a stencil that already extends | |
| (inputs, cards, menu items, buttons, etc.). The one exception is a stencil that already extends |
🧰 Tools
🪛 LanguageTool
[style] ~159-~159: In American English, abbreviations like “etc.” require a period.
Context: ...us (inputs, cards, menu items, buttons, etc). The one exception is a stencil that a...
(ETC_PERIOD)
🤖 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 `@STYLE.md` at line 159, Add the missing period after “etc.” in the
parenthetical phrase so the sentence ends with “etc.).”
Source: Linters/SAST tools
Comment on lines
+221
to
+223
| - Spread `{...stencil.parts.x}` **before** `handleCsProp(elemProps, ...)` so a consumer-supplied | ||
| prop doesn't accidentally override `data-part` unless you intend that. A fixed prop that must win | ||
| (e.g. `variant="secondary"`) goes *after* `handleCsProp` instead. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -A50 -B5 'function handleCsProp' modules/styling/lib/cs.ts
rg -n -C3 'Stencil\.parts\.' modules --glob '*.tsx'Repository: Workday/canvas-kit
Length of output: 31081
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== STYLE.md lines =="
sed -n '200,235p' STYLE.md
echo "== csToProps and handleCsProp complete implementation =="
sed -n '780,940p' modules/styling/lib/cs.ts
echo "== focused real JSX examples around suggested pattern =="
rg -n -C4 '\{\.\.\.handleCsProp\(.*\}\n.*\{\.\.\.stencil\.parts\.' modules --glob '*.tsx' || true
rg -n -C4 '\{\.\.\.stencil\.parts\..*\n.*\{\.\.\.handleCsProp\(.*\}' modules --glob '*.tsx' || trueRepository: Workday/canvas-kit
Length of output: 10396
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== csToProps implementation =="
rg -n -B10 -A24 '^function csToProps|const csToProps|export.*csToProps' modules/styling/lib/cs.ts
echo "== focused JSX examples =="
python3 - <<'PY'
import pathlib
needle1 = '{...stencil.parts.'
needle2 = '{...handleCsProp('
for p in pathlib.Path('modules').rglob('*.tsx'):
text = p.read_text(errors='ignore').splitlines()
for i,line in enumerate(text,1):
if needle2 in line:
# collect next spreads on subsequent lines until comment block or blank-like
j=i+1
while j <= len(text) and (text[j-1].strip().endswith(',') or text[j-1].strip().startswith('{...')):
if needle1 in text[j-1] or needle2 in text[j-1]:
print(f'{p}:{j-1}-{j}: {text[j-1].strip()}')
break
if i-1 > 0 and ('...' in text[j-1] or 'handleCsProp' in text[j-1] or text[j-1].strip().startswith('{')):
continue
break
PY
echo "== simple object-spread semantics probe =="
node - <<'JS'
const stencilPart = {"data-part": "my-part"};
const restProps = {"data-part": "consumer-overridden", "other": "kept"};
const returnProps = {"className": "my-cls", "style": {}, "data-part": "generated"};
console.log(JSON.stringify({
current_order: {...stencilPart, ...returnProps, ...restProps},
reordered_winning_stencil_order: {...returnProps, ...restProps, ...stencilPart}
}, null, 2));
JSRepository: Workday/canvas-kit
Length of output: 148
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== csToProps implementation =="
rg -n -B10 -A24 'csToProps\s*=' modules/styling/lib/cs.ts || true
rg -n -B10 -A24 'csToProps\(' modules/styling/lib/cs.ts || true
rg -n -B10 -A24 'function csToProps|const csToProps' modules/styling/lib/cs.ts || true
echo "== Node object spread behavior =="
node - <<'JS'
const stencilPart = {"data-part": "my-part"};
const returnProps = {"className": "my-cls", "style": {}, "data-part": "generated"};
const restProps = {"data-part": "consumer-overridden", "other": "kept"};
const current_order = {...stencilPart, ...returnProps, ...restProps};
const reordered_winning_stencil_order = {...returnProps, ...restProps, ...stencilPart};
console.log(JSON.stringify({
current_order,
reordered_winning_stencil_order
}, null, 2));
JSRepository: Workday/canvas-kit
Length of output: 6223
Reverse the spread order or update the guarantee.
handleCsProp extracts elemProps['data-part'] into restProps and then spreads restProps at the end. Spreading {...stencil.parts.x} before handleCsProp(elemProps, ...) allows a consumer-supplied data-part to override the stencil part identifier. If the stencil part must win, spread handleCsProp(...) first and {...stencil.parts.x} second.
🤖 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 `@STYLE.md` around lines 221 - 223, Update the prop-spread guidance to match
handleCsProp’s behavior: spread handleCsProp(elemProps, ...) before
stencil.parts.x when the stencil part identifier must take precedence, or revise
the guarantee to explicitly allow consumer data-part overrides. Preserve the
documented ordering for fixed props that must override handleCsProp.
Workday/canvas-kit
 |
||||||||||||||||||||||||||||||||||||||||
| Project |
Workday/canvas-kit
|
| Branch Review |
add-corner-shape-and-parts-conventions
|
| Run status |
|
| Run duration | 02m 32s |
| Commit |
|
| Committer | Sheelah Brennan |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
17
|
 |
0
|
 |
812
|
| View all changes introduced in this branch ↗︎ | |
UI Coverage
19.57%
|
|
|---|---|
|
|
1531
|
|
|
370
|
Accessibility
99.47%
|
|
|---|---|
|
|
5 critical
5 serious
0 moderate
2 minor
|
|
|
72
|
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
Fixes: #4128.
Adds documentation on our
cornerShapestencil and when to use it for CK components. Also adds more details onpartsin stencils, based on what I had in my localCLAUDE.md.Release Category
Documentation
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)