Improve Button styling contract and recipe parity#87
Conversation
… forced-state parity Co-authored-by: bradpotts <4887598+bradpotts@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
WalkthroughThis PR extends the button component to support ARIA attributes and adds dual-token state classes. It updates the recipe to emit both ChangesButton State Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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 docstrings
🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/css-contract.test.ts (1)
205-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winButton contract list is missing
is-*forced-state selector checks.This contract now covers ARIA selectors, but it still doesn’t assert button
is-hover,is-focus, andis-activeselectors that the recipe emits for forced states.Suggested additions to `button variants` selector list
'.sp-btn:focus-visible', + '.sp-btn.is-focus', '.sp-btn.sp-btn--disabled', '.sp-btn[aria-disabled="true"]', '.sp-btn:disabled', '.sp-btn--loading', '.sp-btn[aria-busy="true"]', '.sp-btn--primary:hover', + '.sp-btn--primary.is-hover', + '.sp-btn--primary.is-active', '.sp-btn--primary.sp-btn--disabled', '.sp-btn--primary[aria-disabled="true"]', '.sp-btn--primary:disabled',(Repeat
is-hover/is-activeper variant as needed.)As per coding guidelines, "tests/**: Verify tests cover recipe output correctness, class name generation, and variant exhaustiveness."
🤖 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 `@tests/css-contract.test.ts` around lines 205 - 240, The selectors array in the button contract test is missing forced-state class checks (is-hover/is-focus/is-active) that the recipe emits; update the selectors list (the selectors variable in tests/css-contract.test.ts) to include '.sp-btn.is-hover', '.sp-btn.is-focus', '.sp-btn.is-active' plus matching entries for each variant (e.g., '.sp-btn--primary.is-hover', '.sp-btn--primary.is-focus', '.sp-btn--primary.is-active', and likewise for --secondary, --ghost, --danger, --success, --cta, --accent) so the test asserts the recipe’s forced-state class output for base and per-variant buttons.
🤖 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 `@examples/button-state-verification.html`:
- Around line 42-45: Demo buttons lack an explicit type attribute and will
default to type="submit" in form contexts; update every <button> element used
for demos (e.g., those with class names sp-btn, sp-btn--primary,
sp-btn--secondary, sp-btn--ghost, sp-btn--danger) to include type="button" so
they won’t accidentally submit forms—apply this change to the button groups at
the shown block and the other occurrences noted (lines ~52-56 and ~63-65).
In `@tests/button-recipe.test.ts`:
- Around line 64-66: The three assertions currently check combined substrings
and can false-pass; update the test in tests/button-recipe.test.ts to split the
resulting class string (the variable result) by whitespace into tokens and then
assert exact membership for each token pair: 'sp-btn--hover' and 'is-hover',
'sp-btn--focus' and 'is-focus', and 'sp-btn--active' and 'is-active' (use exact
token checks like tokens.includes('sp-btn--hover') and
tokens.includes('is-hover') rather than toContain on combined substrings).
---
Outside diff comments:
In `@tests/css-contract.test.ts`:
- Around line 205-240: The selectors array in the button contract test is
missing forced-state class checks (is-hover/is-focus/is-active) that the recipe
emits; update the selectors list (the selectors variable in
tests/css-contract.test.ts) to include '.sp-btn.is-hover', '.sp-btn.is-focus',
'.sp-btn.is-active' plus matching entries for each variant (e.g.,
'.sp-btn--primary.is-hover', '.sp-btn--primary.is-focus',
'.sp-btn--primary.is-active', and likewise for --secondary, --ghost, --danger,
--success, --cta, --accent) so the test asserts the recipe’s forced-state class
output for base and per-variant buttons.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: b8bfccdd-1335-4625-ac18-20e5066cfeae
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
examples/button-state-verification.htmlsrc/recipes/button.tssrc/styles/components.csstests/button-recipe.test.tstests/css-contract.test.ts
| <button class="sp-btn sp-btn--primary" aria-busy="true">Primary Loading (Aria)</button> | ||
| <button class="sp-btn sp-btn--secondary" aria-busy="true">Secondary Loading (Aria)</button> | ||
| <button class="sp-btn sp-btn--ghost" aria-busy="true">Ghost Loading (Aria)</button> | ||
| <button class="sp-btn sp-btn--danger" aria-busy="true">Danger Loading (Aria)</button> |
There was a problem hiding this comment.
Add explicit type="button" on all demo buttons.
Without explicit type, buttons default to submit in form contexts and can create misleading verification behavior.
Suggested fix
- <button class="sp-btn sp-btn--primary" aria-busy="true">Primary Loading (Aria)</button>
+ <button type="button" class="sp-btn sp-btn--primary" aria-busy="true">Primary Loading (Aria)</button>Apply the same to each <button> in this file.
Also applies to: 52-56, 63-65
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 42-42: The type attribute must be present on elements.
(button-type-require)
[warning] 43-43: The type attribute must be present on
elements.(button-type-require)
[warning] 44-44: The type attribute must be present on
elements.(button-type-require)
[warning] 45-45: The type attribute must be present on
elements.(button-type-require)
🤖 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 `@examples/button-state-verification.html` around lines 42 - 45, Demo buttons
lack an explicit type attribute and will default to type="submit" in form
contexts; update every <button> element used for demos (e.g., those with class
names sp-btn, sp-btn--primary, sp-btn--secondary, sp-btn--ghost, sp-btn--danger)
to include type="button" so they won’t accidentally submit forms—apply this
change to the button groups at the shown block and the other occurrences noted
(lines ~52-56 and ~63-65).
| expect(result).toContain('sp-btn--hover is-hover'); | ||
| expect(result).toContain('sp-btn--focus is-focus'); | ||
| expect(result).toContain('sp-btn--active is-active'); |
There was a problem hiding this comment.
Assert state tokens individually instead of combined substrings.
These checks can false-pass on partial matches. Split by whitespace and assert exact token membership for both sp-btn--* and is-* tokens.
Suggested test hardening
- expect(result).toContain('sp-btn--hover is-hover');
- expect(result).toContain('sp-btn--focus is-focus');
- expect(result).toContain('sp-btn--active is-active');
+ const tokens = result.split(/\s+/)
+ expect(tokens).toContain('sp-btn--hover')
+ expect(tokens).toContain('is-hover')
+ expect(tokens).toContain('sp-btn--focus')
+ expect(tokens).toContain('is-focus')
+ expect(tokens).toContain('sp-btn--active')
+ expect(tokens).toContain('is-active')As per coding guidelines, "tests/**: Verify tests cover recipe output correctness, class name generation, and variant exhaustiveness."
📝 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.
| expect(result).toContain('sp-btn--hover is-hover'); | |
| expect(result).toContain('sp-btn--focus is-focus'); | |
| expect(result).toContain('sp-btn--active is-active'); | |
| const tokens = result.split(/\s+/) | |
| expect(tokens).toContain('sp-btn--hover') | |
| expect(tokens).toContain('is-hover') | |
| expect(tokens).toContain('sp-btn--focus') | |
| expect(tokens).toContain('is-focus') | |
| expect(tokens).toContain('sp-btn--active') | |
| expect(tokens).toContain('is-active') |
🤖 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 `@tests/button-recipe.test.ts` around lines 64 - 66, The three assertions
currently check combined substrings and can false-pass; update the test in
tests/button-recipe.test.ts to split the resulting class string (the variable
result) by whitespace into tokens and then assert exact membership for each
token pair: 'sp-btn--hover' and 'is-hover', 'sp-btn--focus' and 'is-focus', and
'sp-btn--active' and 'is-active' (use exact token checks like
tokens.includes('sp-btn--hover') and tokens.includes('is-hover') rather than
toContain on combined substrings).
1 participant
This improvement enhances the Button component's styling contract by adding support for standard ARIA attributes (
aria-busyandaria-disabled) across all variants, ensuring accessibility and state parity. Additionally, the Button recipe was updated to include forced-state classes (.is-hover,.is-focus,.is-active), aligning it with the design system's interactive pattern. Tests and visual verification examples were updated to confirm the changes.PR created automatically by Jules for task 14676055231159547280 started by @bradpotts
Summary by CodeRabbit
New Features
Improvements