Improve PricingCard UI contract and state parity#81
Conversation
This change updates the PricingCard component to support standard interactive states and accessibility attributes. - Updated `src/recipes/pricing-card.ts` to include forced-state classes (`is-hover`, `is-focus`, `is-active`) in the recipe output for parity with other components. - Updated `src/styles/components.css` to add support for `:disabled`, `[aria-disabled="true"]`, and `[aria-busy="true"]` selectors. - Updated `tests/pricing-card-recipe.test.ts` and `tests/css-contract.test.ts` to verify the new contract mappings. - Verified visual changes via Playwright screenshot. 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. |
WalkthroughPricing card component and styles updated to emit and respond to ARIA state attributes alongside class-based state indicators. Recipe function now outputs ChangesPricing Card ARIA State Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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 `@src/styles/components.css`:
- Around line 1228-1229: Remove the ineffective pseudo-class selector by
deleting `:disabled` from the selector group so `.sp-pricing-card:disabled` is
not used; keep and rely on the attribute selector
`.sp-pricing-card[aria-disabled="true"]` and the modifier class
`.sp-pricing-card--disabled` for disabled styling, and update any grouped
selector lists that currently include `.sp-pricing-card:disabled` to only
reference the valid selectors (e.g., `.sp-pricing-card[aria-disabled="true"]`,
`.sp-pricing-card--disabled`).
🪄 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: 8e7028c9-1772-4258-b473-e8455cf02eb1
📒 Files selected for processing (5)
examples/pricing-card-verification.htmlsrc/recipes/pricing-card.tssrc/styles/components.csstests/css-contract.test.tstests/pricing-card-recipe.test.ts
| .sp-pricing-card:disabled, | ||
| .sp-pricing-card[aria-disabled="true"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if pricing cards are implemented as form elements or divs
# Search for pricing card HTML structure in examples and documentation
rg -n -C2 'sp-pricing-card' --type=html
# Also check for any button or fieldset usage with pricing card class
rg -nP '(?:<button|<fieldset)[^>]*sp-pricing-card' --type=htmlRepository: phcdevworks/spectre-ui
Length of output: 13524
🏁 Script executed:
sed -n '1220,1240p' src/styles/components.cssRepository: phcdevworks/spectre-ui
Length of output: 603
Remove the ineffective :disabled pseudo-class selector from the pricing card rule.
The .sp-pricing-card:disabled selector on line 1228 will never match because pricing cards are <div> elements. The :disabled pseudo-class only applies to form-associated elements like <button>, <input>, <select>, <textarea>, and <fieldset>. The [aria-disabled="true"] attribute selector and .sp-pricing-card--disabled class modifier on the following lines provide the correct coverage for non-form elements.
Proposed fix
-.sp-pricing-card:disabled,
.sp-pricing-card[aria-disabled="true"],
.sp-pricing-card--disabled {📝 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.
| .sp-pricing-card:disabled, | |
| .sp-pricing-card[aria-disabled="true"], | |
| .sp-pricing-card[aria-disabled="true"], | |
| .sp-pricing-card--disabled { |
🤖 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 `@src/styles/components.css` around lines 1228 - 1229, Remove the ineffective
pseudo-class selector by deleting `:disabled` from the selector group so
`.sp-pricing-card:disabled` is not used; keep and rely on the attribute selector
`.sp-pricing-card[aria-disabled="true"]` and the modifier class
`.sp-pricing-card--disabled` for disabled styling, and update any grouped
selector lists that currently include `.sp-pricing-card:disabled` to only
reference the valid selectors (e.g., `.sp-pricing-card[aria-disabled="true"]`,
`.sp-pricing-card--disabled`).
Address missing standard states and ensure parity between the recipe and CSS. - Updated `src/recipes/pricing-card.ts` to include forced-state classes (`is-hover`, `is-focus`, `is-active`) in the recipe output. - Updated `src/styles/components.css` to add support for `:disabled`, `[aria-disabled="true"]`, and `[aria-busy="true"]` selectors, mapping them to design tokens. - Bridged the forced-state classes in `src/styles/components.css` to allow programmatic state overrides. - Updated `tests/pricing-card-recipe.test.ts` and `tests/css-contract.test.ts` to reflect the contract improvements. Note: GitHub CI may report EBADENGINE due to the runner's npm version (10.9.2) being lower than the project requirement (>=11.0.0). Local validation with npm 11.11.0 confirms the fix. Co-authored-by: bradpotts <4887598+bradpotts@users.noreply.github.com>
Address missing standard states and ensure parity between the recipe and CSS. - Updated `src/recipes/pricing-card.ts` to include forced-state classes (`is-hover`, `is-focus`, `is-active`) in the recipe output. - Updated `src/styles/components.css` to add support for `:disabled`, `[aria-disabled="true"]`, and `[aria-busy="true"]` selectors, mapping them to design tokens. - Bridged the forced-state classes in `src/styles/components.css` to allow programmatic state overrides. - Updated `tests/pricing-card-recipe.test.ts` and `tests/css-contract.test.ts` to reflect the contract improvements. Note: GitHub CI may report EBADENGINE due to the runner's npm version (10.9.2) being lower than the project requirement (>=11.0.0). Local validation with npm 11.11.0 confirms the fix. Co-authored-by: bradpotts <4887598+bradpotts@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/styles/components.css (1)
1228-1228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the ineffective
:disabledpseudo-class selector.The
.sp-pricing-card:disabledselector will never match because the:disabledpseudo-class only applies to form-associated elements (<button>,<input>,<select>,<textarea>,<fieldset>), not<div>elements. The[aria-disabled="true"]attribute selector (line 1229) and.sp-pricing-card--disabledclass modifier (line 1230) already provide the correct coverage for disabled states.Proposed fix
-.sp-pricing-card:disabled, .sp-pricing-card[aria-disabled="true"], .sp-pricing-card--disabled {As per coding guidelines, CSS classes and recipe APIs must remain in sync at all times; removing this non-functional selector eliminates contract drift.
🤖 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 `@src/styles/components.css` at line 1228, Remove the ineffective pseudo-class selector .sp-pricing-card:disabled and rely on the existing [aria-disabled="true"] attribute selector and .sp-pricing-card--disabled modifier for disabled styling; update the CSS by deleting the .sp-pricing-card:disabled entry wherever it appears (so only [aria-disabled="true"] and .sp-pricing-card--disabled remain) to keep class/recipe API contract in sync with the styling rules.
🤖 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 `@tests/css-contract.test.ts`:
- Line 280: Remove the non-functional selector ".sp-pricing-card:disabled" from
the test's expected selectors so the contract no longer asserts a :disabled
pseudo-class on a non-form element; locate the array or assertion in
tests/css-contract.test.ts that includes the string ".sp-pricing-card:disabled"
(the expected selectors/validation block) and delete that entry, then update any
related snapshot/assertion to reflect the corrected expected output.
---
Duplicate comments:
In `@src/styles/components.css`:
- Line 1228: Remove the ineffective pseudo-class selector
.sp-pricing-card:disabled and rely on the existing [aria-disabled="true"]
attribute selector and .sp-pricing-card--disabled modifier for disabled styling;
update the CSS by deleting the .sp-pricing-card:disabled entry wherever it
appears (so only [aria-disabled="true"] and .sp-pricing-card--disabled remain)
to keep class/recipe API contract in sync with the styling rules.
🪄 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: c4838771-416a-47a4-a188-2755daf58afb
📒 Files selected for processing (2)
src/styles/components.csstests/css-contract.test.ts
| '.sp-pricing-card--interactive:hover', | ||
| '.sp-pricing-card--interactive:focus-visible', | ||
| '.sp-pricing-card--interactive:focus-within', | ||
| '.sp-pricing-card:disabled', |
There was a problem hiding this comment.
Remove validation for the non-functional :disabled selector.
This test validates the presence of .sp-pricing-card:disabled, but this selector is non-functional because the :disabled pseudo-class only applies to form elements, not <div> elements. The test creates a false sense of contract coverage.
Proposed fix
'.sp-pricing-card--interactive:hover',
'.sp-pricing-card--interactive:focus-visible',
'.sp-pricing-card--interactive:focus-within',
- '.sp-pricing-card:disabled',
'.sp-pricing-card[aria-disabled="true"]',
'.sp-pricing-card--disabled',As per coding guidelines, tests should verify recipe output correctness and avoid validating incorrect output.
📝 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.
| '.sp-pricing-card:disabled', | |
| '.sp-pricing-card--interactive:hover', | |
| '.sp-pricing-card--interactive:focus-visible', | |
| '.sp-pricing-card--interactive:focus-within', | |
| '.sp-pricing-card[aria-disabled="true"]', | |
| '.sp-pricing-card--disabled', |
🤖 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` at line 280, Remove the non-functional selector
".sp-pricing-card:disabled" from the test's expected selectors so the contract
no longer asserts a :disabled pseudo-class on a non-form element; locate the
array or assertion in tests/css-contract.test.ts that includes the string
".sp-pricing-card:disabled" (the expected selectors/validation block) and delete
that entry, then update any related snapshot/assertion to reflect the corrected
expected output.
Address missing standard states and ensure parity between the recipe and CSS. - Updated `src/recipes/pricing-card.ts` to include forced-state classes (`is-hover`, `is-focus`, `is-active`) in the recipe output. - Updated `src/styles/components.css` to add support for `:disabled`, `[aria-disabled="true"]`, and `[aria-busy="true"]` selectors, mapping them to design tokens. - Bridged the forced-state classes in `src/styles/components.css` to allow programmatic state overrides. - Updated `tests/pricing-card-recipe.test.ts` and `tests/css-contract.test.ts` to reflect the contract improvements. Note: GitHub CI may report EBADENGINE due to the runner's npm version (10.9.2) being lower than the project requirement (>=11.0.0). Local validation with `npm run ci:verify` (npm 11.11.0) passes clean. Co-authored-by: bradpotts <4887598+bradpotts@users.noreply.github.com>
There was a problem hiding this comment.
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)
276-287:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd forced-state class selectors to the pricing card contract.
The CSS file contains
.sp-pricing-card.is-hover,.sp-pricing-card.is-focus,.sp-pricing-card.is-activeand their--featuredvariants (lines 1252–1283 in src/styles/components.css), but the contract test only validates pseudo-class and ARIA attribute selectors. Add the following to theselectorsarray:'.sp-pricing-card.is-hover', '.sp-pricing-card.is-focus', '.sp-pricing-card.is-active', '.sp-pricing-card--featured.is-hover', '.sp-pricing-card--featured.is-focus', '.sp-pricing-card--featured.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/css-contract.test.ts` around lines 276 - 287, The contract test's selectors array (the one containing entries like '.sp-pricing-card--interactive:hover', '.sp-pricing-card--interactive:focus-visible', '.sp-pricing-card:disabled', etc.) is missing the forced-state class selectors present in the CSS; update that selectors array to also include '.sp-pricing-card.is-hover', '.sp-pricing-card.is-focus', '.sp-pricing-card.is-active' and the featured variants '.sp-pricing-card--featured.is-hover', '.sp-pricing-card--featured.is-focus', '.sp-pricing-card--featured.is-active' so the test validates the CSS forced-state classes as well.
🤖 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.
Outside diff comments:
In `@tests/css-contract.test.ts`:
- Around line 276-287: The contract test's selectors array (the one containing
entries like '.sp-pricing-card--interactive:hover',
'.sp-pricing-card--interactive:focus-visible', '.sp-pricing-card:disabled',
etc.) is missing the forced-state class selectors present in the CSS; update
that selectors array to also include '.sp-pricing-card.is-hover',
'.sp-pricing-card.is-focus', '.sp-pricing-card.is-active' and the featured
variants '.sp-pricing-card--featured.is-hover',
'.sp-pricing-card--featured.is-focus', '.sp-pricing-card--featured.is-active' so
the test validates the CSS forced-state classes as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2a9c5441-4f34-4621-8420-025736a53309
📒 Files selected for processing (1)
tests/css-contract.test.ts
1 participant
I have improved the
PricingCardcomponent's UI contract by adding support for standard interactive states (hover,focus,active) using the forced-state pattern (is-hover, etc.) and standard accessibility attributes (aria-disabled,aria-busy).Key changes:
src/recipes/pricing-card.tsnow bridges boolean state flags to both component-specific modifiers and globalis-*forced-state classes.src/styles/components.cssnow includes selectors for:disabled,[aria-disabled="true"], and[aria-busy="true"]for thePricingCardcomponent, mapping them to standard opacity tokens.PR created automatically by Jules for task 2267319964026000406 started by @bradpotts
Summary by CodeRabbit
Style
Tests