-
Notifications
You must be signed in to change notification settings - Fork 229
Testing: button refactor #5587
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.
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
base: main
Are you sure you want to change the base?
Testing: button refactor #5587
Conversation
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
|
placement="top" | ||
variant="info" | ||
offset="6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be configured.
Refactor: Enhanced Accessibility, Static Types, and Disabled Reason Tooltip for Button
Overview
This PR introduces a set of improvements and refactors to the
<sp-button>
component and its base class, focused on accessibility, developer ergonomics, and user experience. The changes aim to make button states clearer and more robust, particularly around disabled and pending states.Changes Done
1. Type Improvements
as const
toVALID_VARIANTS
andVALID_STATIC_COLORS
to enforce stronger static typing for these value arrays.2. Accessibility Enhancements
role="button"
by default if not already present.aria-disabled
attribute is dynamically updated in response to the button'sdisabled
state.aria-busy
attribute is dynamically updated when the button is in apending
state.3. Pending State Handling
pending
property is now consistently overridden in theButton
class and handled with the proper decorator for reflection.click()
method now prevents interaction when the button is eitherpending
ordisabled
.4. Improved Disabled State Logic
disabledReason
property. When set, the button will:aria-disabled
(instead of the nativedisabled
attribute), keeping the button focusable for better accessibility.sp-tooltip
) with the provided reason when hovered.isDisabled
getter to centralize logic for determining if the button is disabled (either viadisabled
ordisabledReason
).5. Tooltip Integration
disabledReason
is present, ansp-tooltip
is rendered above the button with the provided message to communicate to users why the button is disabled.6. General Cleanups
isDisabled
logic.Problem Solved
These changes address several usability and accessibility issues:
aria-disabled
,aria-busy
) and always providing arole
, screen readers and assistive technologies can better interpret the button’s state.disabledReason
property allows developers to communicate to users why an action is unavailable, improving user experience through contextual tooltips.as const
and improved property typing results in safer, more predictable code, reducing potential bugs around allowed values for variants and static colors.Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch
,minor
, ormajor
featuresManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review