Skip to content
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

[EuiButtonIcon] Add isLoading prop to match EuiButton #5649

Closed
p-young opened this issue Feb 17, 2022 · 3 comments · Fixed by #5668
Closed

[EuiButtonIcon] Add isLoading prop to match EuiButton #5649

p-young opened this issue Feb 17, 2022 · 3 comments · Fixed by #5668
Assignees

Comments

@p-young
Copy link

p-young commented Feb 17, 2022

This is a feature request to allow EuiButtonIcon to also have a loading state "isLoading" like EuiButton does.

The use case is for example a chat submit button, which needs to be put into a loading state after submission.

A workaround is to use EuiButton with an icon and manually change the margins to make it look like an EuiButtonIcon, but I think it would be nice to have EuiButtonIcon directly do it.

Thanks!

@elizabetdev
Copy link
Contributor

Thanks, @p-young for opening this issue.

The use case sounds good and we are going to consider it.

I have one suggestion for your workaround. Instead of using a EuiButton with an icon, you can use instead a EuiLoadingSpinner: https://codesandbox.io/s/euibuttonicon-loading-0ffczp?file=/demo.js:378-395.

@elizabetdev elizabetdev self-assigned this Feb 22, 2022
@elizabetdev
Copy link
Contributor

To introduce a EuiButtonIcon isLoading state I can follow the other buttons isLoading state approach:

  • The button gets disabled.
  • The EuiIcon gets replaced with a EuiLoadingSpinner.
  • We introduce a new size to EuiLoadingSpinner "XXL" so that all the sizes match the EuiIcon sizes.

But I have some a11y questions.

  • When the button gets disabled the focus state is missed.
  • It gets removed from the accessibility tree and is invisible to assistive technology users

So when the EuiButtonIcon isLoading would adding an aria-disabled instead of disabled be more appropriate (link)? Or is it OK to keep following the other buttons' pattern?

CC @1Copenut @cchaos

@1Copenut
Copy link
Contributor

@miukimiu & @cchaos

I've been thinking about this for a bit. I think EuiButtonIcon isLoading state should have the same behavior as other buttons, for now. It could be more disruptive to have one button that handles the disabled state differently than others.

With that said, I think it would benefit us to review our disabled button pattern for all buttons and look at the effort to move to a button[aria-disbled="true"] model. I'll add a backlog research ticket to EUI to track that longer time horizon work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants