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

feat(Button): use LoadingIndicator for loading state #1552

Merged
merged 1 commit into from
Mar 20, 2023

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented Mar 20, 2023

Summary:

  • replace the inline icon (loading) with our
  • remove custom animation as well
  • update snapshots to account for this
  • don't use the visible prop, as this breaks ARIA for buttons
  • mark loading as deprecated (we will direct people and designers to compose instead, like we do with icons)

Test Plan:

  • update snapshots

@booc0mtaco booc0mtaco requested a review from a team March 20, 2023 21:58
@github-actions
Copy link

github-actions bot commented Mar 20, 2023

size-limit report 📦

Path Size
components 121.42 KB (-0.03% 🔽)
styles 3.1 KB (0%)

- replace the inline icon (loading) with our <LoadingIndicator />
- update snapshots to account for this
- don't use the visible prop, as this breaks ARIA for buttons
- marking prop as deprecated since it can be handled thru composition
  (like arrow)
/>
)}

{loading && <LoadingIndicator className="eds-is-loading" size="sm" />}
Copy link
Contributor

Choose a reason for hiding this comment

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

When do you think this variant will be removed? Just not a fan of this eds-is-loading classnaming haha should be more BEM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's chat about it this week. we could do whenever, but removing is technically breaking, so didn't want to commit to a v12 just yet :)

agreed on that class and this implementation 🪓

Copy link
Contributor

@jinlee93 jinlee93 left a comment

Choose a reason for hiding this comment

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

Looks good, just one probably unrelated to landing q

@booc0mtaco
Copy link
Contributor Author

@jinlee93 Thanks! we can chat this week to see what the coming week has in store w.r.t. publishable changes

@booc0mtaco booc0mtaco merged commit 2406d01 into next Mar 20, 2023
@booc0mtaco booc0mtaco deleted the aholloway/EDS-778 branch March 20, 2023 23:38
@booc0mtaco booc0mtaco mentioned this pull request Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants