Persist busy state on hover #61276
Closed
Persist busy state on hover #61276
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jameskoster
added
[Type] Enhancement
|
Size Change: +68 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
t-hamano
reviewed
May 1, 2024
There was a problem hiding this comment.
If isBusy is true, how about setting disabled to true at the same time? This approach is already widely used and does not require changing the behavior of the Button component itself. The hover action will also be ignored.
<Button
isBusy={ isBusy }
disabled={ isBusy }
>
{ __( 'Delete' ) }
</Button>
|
So long as we're happy for busy buttons to be non-interactive that might work. Though, I suppose it would have to be |
|
My understanding is that opting in to Not this: <Button
aria-disabled={ isBusy }
isBusy={ isBusy }
/>like this: <Button
disabled={ isBusy }
isBusy={ isBusy }
__experimentalIsFocusable
/>Pinging @WordPress/gutenberg-components for confirmation just in case 🙏 |
|
There's may be a better way, but I pushed a commit which applies |
|
@WordPress/gutenberg-components does someone have a moment to review this one? :) |
tyxla
reviewed
May 8, 2024
| @@ -174,7 +174,7 @@ export function UnforwardedButton( | |||
| const anchorProps: ComponentPropsWithoutRef< 'a' > = | |||
| Tag === 'a' ? { href, target } : {}; | |||
|
|
|||
| if ( disabled && isFocusable ) { | |||
| if ( ( disabled && isFocusable ) || isBusy ) { | |||
There was a problem hiding this comment.
I disagree with this change. IMHO you might want to keep the button appearing busy, but still allow it to be non-disabled. If you really want to disable it, we do have the disabled prop that allows you to do it.
| @@ -75,7 +75,7 @@ | |||
| &:disabled:active:enabled, | |||
| &[aria-disabled="true"], | |||
| &[aria-disabled="true"]:enabled, // This catches a situation where a Button is aria-disabled, but not disabled. | |||
| &[aria-disabled="true"]:active:enabled { | |||
| &[aria-disabled="true"]:active:enabled:not(.is-busy) { | |||
There was a problem hiding this comment.
This works as intended and no longer changes the hover state of busy buttons.
However, I'm not sure I agree with it. I think busy buttons should still have a hover state, and that hover state should just respect the busy button styles.
Although most component libraries make loading/busy buttons disabled, I can definitely see a valid case for a busy button that's still not disabled. Maybe an optimistic "Add to Cart" button that remains enabled? Or any other counter-like button that counts something inline but also submits data to the server? We also have __experimentalIsFocusable so that fixes the a11y concerns about disabled busy buttons.
In simple terms, I'd expect if the button is not disabled but is busy, to have a hover state - darker/lighter color, but still appearing busy. Then if the busy button is also disabled, no hover style at all.
There was a problem hiding this comment.
Fair point. I'm not quite grasping the examples, but I suppose allowing consumers to employ enabled & busy buttons is more flexible.
For the example in the OP I guess the solution would be to make the Button busy and disabled.
The styling of primary busy buttons might use a tweak, but that's one to look at separately.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This came up in #59714.
What?
Ensure 'busy' styling remains visible when hovering busy buttons.
Why?
When clicking a button involves immediately invoking the busy state, you cannot currently see that the button is busy because the hover styles override the busy styles:
busy.mp4
How?
Do not apply hover styles when the button
is-busy:buttons.mp4
Testing Instructions
npm run storybook:devbusypropSidenote
Should busy buttons still show the
pointercursor on hover? That seems a bit unexpected to me.