Skip to content

Improve Button styling contract and recipe parity #87

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bradpotts merged 1 commit into from
May 22, 2026
Merged

Improve Button styling contract and recipe parity #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions examples/button-state-verification.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Button State Verification</title>
<link rel="stylesheet" href="../dist/index.css">
<style>
body {
padding: 2rem;
font-family: sans-serif;
display: flex;
flex-direction: column;
gap: 2rem;
}
section {
display: flex;
flex-direction: column;
gap: 1rem;
border: 1px solid #ccc;
padding: 1rem;
border-radius: 8px;
}
.row {
display: flex;
gap: 1rem;
align-items: center;
flex-wrap: wrap;
}
h2 {
margin: 0;
font-size: 1.2rem;
}
</style>
</head>
<body>
<h1>Button State Verification</h1>

<section>
<h2>Aria-Busy (Loading) State</h2>
<div class="row">
<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>
Comment on lines +42 to +45
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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).

</div>
</section>

<section>
<h2>Aria-Disabled State</h2>
<div class="row">
<button class="sp-btn sp-btn--primary" aria-disabled="true">Primary Disabled (Aria)</button>
<button class="sp-btn sp-btn--secondary" aria-disabled="true">Secondary Disabled (Aria)</button>
<button class="sp-btn sp-btn--ghost" aria-disabled="true">Ghost Disabled (Aria)</button>
<button class="sp-btn sp-btn--cta" aria-disabled="true">CTA Disabled (Aria)</button>
<button class="sp-btn sp-btn--accent" aria-disabled="true">Accent Disabled (Aria)</button>
</div>
</section>

<section>
<h2>Forced States (Recipe Output Verification)</h2>
<div class="row">
<button class="sp-btn sp-btn--primary is-hover">Primary Hovered</button>
<button class="sp-btn sp-btn--secondary is-focus">Secondary Focused</button>
<button class="sp-btn sp-btn--danger is-active">Danger Active</button>
</div>
</section>

</body>
</html>
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions src/recipes/button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ export function getButtonClasses(opts: ButtonRecipeOptions = {}): string {
fullWidth && "sp-btn--full",
loading && "sp-btn--loading",
disabled && "sp-btn--disabled",
hovered && "sp-btn--hover",
focused && "sp-btn--focus",
active && "sp-btn--active",
hovered && "sp-btn--hover is-hover",
focused && "sp-btn--focus is-focus",
active && "sp-btn--active is-active",
iconOnly && "sp-btn--icon",
pill && "sp-btn--pill",
);
Expand Down
10 changes: 9 additions & 1 deletion src/styles/components.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,8 @@
pointer-events: none;
}

.sp-btn--loading {
.sp-btn--loading,
.sp-btn[aria-busy="true"] {
pointer-events: none;
opacity: var(--sp-opacity-active);
}
Expand Down Expand Up @@ -338,6 +339,7 @@
}

.sp-btn--primary.sp-btn--disabled,
.sp-btn--primary[aria-disabled="true"],
.sp-btn--primary:disabled {
background-color: var(--sp-component-button-primary-bg-disabled);
color: var(--sp-component-button-primary-text-disabled);
Expand All @@ -364,6 +366,7 @@
}

.sp-btn--secondary.sp-btn--disabled,
.sp-btn--secondary[aria-disabled="true"],
.sp-btn--secondary:disabled {
background-color: var(--sp-component-button-secondary-bg-disabled);
color: var(--sp-component-button-secondary-text-disabled);
Expand All @@ -389,6 +392,7 @@
}

.sp-btn--ghost.sp-btn--disabled,
.sp-btn--ghost[aria-disabled="true"],
.sp-btn--ghost:disabled {
color: var(--sp-component-button-ghost-text-disabled);
background-color: var(--sp-component-button-ghost-bg-disabled);
Expand All @@ -413,6 +417,7 @@
}

.sp-btn--danger.sp-btn--disabled,
.sp-btn--danger[aria-disabled="true"],
.sp-btn--danger:disabled {
background-color: var(--sp-component-button-danger-bg-disabled);
color: var(--sp-component-button-danger-text-disabled);
Expand All @@ -437,6 +442,7 @@
}

.sp-btn--success.sp-btn--disabled,
.sp-btn--success[aria-disabled="true"],
.sp-btn--success:disabled {
background-color: var(--sp-component-button-success-bg-disabled);
color: var(--sp-component-button-success-text-disabled);
Expand All @@ -462,6 +468,7 @@
}

.sp-btn--cta.sp-btn--disabled,
.sp-btn--cta[aria-disabled="true"],
.sp-btn--cta:disabled {
background-color: var(--sp-component-button-cta-bg-disabled);
color: var(--sp-component-button-cta-text-disabled);
Expand All @@ -487,6 +494,7 @@
}

.sp-btn--accent.sp-btn--disabled,
.sp-btn--accent[aria-disabled="true"],
.sp-btn--accent:disabled {
background-color: var(--sp-component-button-accent-bg-disabled);
color: var(--sp-component-button-accent-text-disabled);
Expand Down
6 changes: 3 additions & 3 deletions tests/button-recipe.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ describe('getButtonClasses', () => {
expect(result).toContain('sp-btn--full');
expect(result).toContain('sp-btn--loading');
expect(result).toContain('sp-btn--disabled');
expect(result).toContain('sp-btn--hover');
expect(result).toContain('sp-btn--focus');
expect(result).toContain('sp-btn--active');
expect(result).toContain('sp-btn--hover is-hover');
expect(result).toContain('sp-btn--focus is-focus');
expect(result).toContain('sp-btn--active is-active');
Comment on lines +64 to +66
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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).

expect(result).toContain('sp-btn--icon');
expect(result).toContain('sp-btn--pill');
});
Expand Down
16 changes: 16 additions & 0 deletions tests/css-contract.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,19 +207,35 @@ const interactionStateContracts = [
'.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.sp-btn--disabled',
'.sp-btn--primary[aria-disabled="true"]',
'.sp-btn--primary:disabled',
'.sp-btn--secondary:hover',
'.sp-btn--secondary.sp-btn--disabled',
'.sp-btn--secondary[aria-disabled="true"]',
'.sp-btn--secondary:disabled',
'.sp-btn--ghost:hover',
'.sp-btn--ghost.sp-btn--disabled',
'.sp-btn--ghost[aria-disabled="true"]',
'.sp-btn--ghost:disabled',
'.sp-btn--danger:hover',
'.sp-btn--danger.sp-btn--disabled',
'.sp-btn--danger[aria-disabled="true"]',
'.sp-btn--danger:disabled',
'.sp-btn--success:hover',
'.sp-btn--success.sp-btn--disabled',
'.sp-btn--success[aria-disabled="true"]',
'.sp-btn--success:disabled',
'.sp-btn--cta:hover',
'.sp-btn--cta.sp-btn--disabled',
'.sp-btn--cta[aria-disabled="true"]',
'.sp-btn--cta:disabled',
'.sp-btn--accent:hover',
'.sp-btn--accent.sp-btn--disabled',
'.sp-btn--accent[aria-disabled="true"]',
'.sp-btn--accent:disabled',
],
},
Expand Down
Loading