Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds styling support for tertiary danger buttons, extending the existing button variant system with a new combination of tertiary styling and danger semantics.
Key Changes
- Added CSS custom properties for tertiary danger buttons in both light and dark themes
- Implemented tertiary danger button styles with hover and active states
- Updated CHANGELOG to document the new feature
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/assets/stylesheets/index.scss | Adds CSS custom properties for tertiary danger button colors in both dark and light themes, defining text color and background colors for hover/active states |
| src/assets/stylesheets/Button.scss | Implements the tertiary danger button variant nested within the danger button modifier, applying the new CSS variables with hover and active state handling |
| CHANGELOG.md | Documents the addition of tertiary danger button styles under the Unreleased section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| &:hover { | ||
| background-color: var(--rpf-button-tertiary-danger-background-color-hover); | ||
| } | ||
|
|
||
| &:active { | ||
| background-color: var(--rpf-button-tertiary-danger-background-color-active); | ||
| } |
There was a problem hiding this comment.
The tertiary danger button implementation is missing several important interactive states that are present in other button variants:
-
Missing
:focus-visiblestate: Other button variants (primary, secondary, tertiary, and danger) all have:focus-visiblestyles. The tertiary danger button should also handle focus visibility for accessibility. -
Missing
:disabledstate: The base tertiary button (lines 156-168) includes a:disabledstate with specific styling. The tertiary danger button should also handle the disabled state. -
Missing
.btn-outerwrapper selectors: Other button states use both direct pseudo-classes (e.g.,&:hover) and wrapper-based selectors (e.g.,.btn-outer:hover &) for hover, active, and focus-visible states. The tertiary danger button should follow this same pattern for consistency.
Consider adding these missing states to ensure consistent behavior with other button variants.
| &:hover { | |
| background-color: var(--rpf-button-tertiary-danger-background-color-hover); | |
| } | |
| &:active { | |
| background-color: var(--rpf-button-tertiary-danger-background-color-active); | |
| } | |
| // Hover states | |
| &:hover, | |
| .btn-outer:hover & { | |
| background-color: var(--rpf-button-tertiary-danger-background-color-hover); | |
| } | |
| // Active states | |
| &:active, | |
| .btn-outer:active & { | |
| background-color: var(--rpf-button-tertiary-danger-background-color-active); | |
| } | |
| // Focus-visible states | |
| &:focus-visible, | |
| .btn-outer:focus-visible & { | |
| background-clip: padding-box; | |
| border: 2px solid transparent; | |
| outline: 3px solid $rpf-brand-raspberry; | |
| } | |
| // Disabled states | |
| &:disabled { | |
| background-color: inherit; | |
| color: var(--rpf-button-tertiary-danger-text-color); | |
| cursor: default; | |
| svg { | |
| fill: var(--rpf-button-tertiary-danger-text-color); | |
| } | |
| &:hover, | |
| .btn-outer:hover & { | |
| background-color: inherit; | |
| } | |
| } |
There was a problem hiding this comment.
We're using the default focus state for this. We have no use case currently for disabling this and we're not using btn-outer here, so there's no need to add these, and they can be included at a future date if the need arises.
What's Changed?
related to https://github.com/RaspberryPiFoundation/digital-editor-issues/issues/948