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

Allow IconButton to receive loading props #1570

Merged
merged 4 commits into from
May 17, 2022
Merged

Conversation

vascofg
Copy link
Contributor

@vascofg vascofg commented May 5, 2022

Purpose

Allow IconButton to display a loading state.

Approach and changes

Update types to allow isLoading and loadingLabel props for IconButton.
The behaviour already works from the parent Button component.

Definition of done

  • Development completed
  • Reviewers assigned
  • Unit and integration tests
  • Meets minimum browser support
  • Meets accessibility requirements

@vascofg vascofg requested a review from a team as a code owner May 5, 2022 08:03
@vascofg vascofg requested review from robinmetral and removed request for a team May 5, 2022 08:03
@sumup-clark
Copy link

sumup-clark bot commented May 5, 2022

Hey @vascofg,
we are super excited that you are contributing! 🎉There's one more thing you need to do. Please accept our Contributor License Agreement. It helps you and us to collaborate on clear terms and focus on what we love most: code.

Thanks!

@vercel
Copy link

vercel bot commented May 5, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
oss-circuit-ui ✅ Ready (Inspect) Visit Preview May 10, 2022 at 6:43PM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented May 5, 2022

🦋 Changeset detected

Latest commit: cc32272

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sumup/circuit-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vascofg vascofg self-assigned this May 5, 2022
@vascofg vascofg changed the title Allow IconButton to received loading props Allow IconButton to receive loading props May 5, 2022
Copy link
Contributor

@sumius sumius left a comment

Choose a reason for hiding this comment

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

Good to me!

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #1570 (cc32272) into main (3f32536) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1570   +/-   ##
=======================================
  Coverage   92.71%   92.71%           
=======================================
  Files         194      194           
  Lines        4024     4024           
  Branches     1262     1262           
=======================================
  Hits         3731     3731           
  Misses        274      274           
  Partials       19       19           
Impacted Files Coverage Δ
...es/circuit-ui/components/IconButton/IconButton.tsx 88.88% <ø> (ø)

@robinmetral
Copy link
Contributor

Hey @vascofg, thanks for the PR! Can you explain what you're trying to achieve a bit more? Since the prop types extend ButtonProps, as you mentioned, the loading state should already be supported. Isn't it?

@vascofg
Copy link
Contributor Author

vascofg commented May 10, 2022

Hey @vascofg, thanks for the PR! Can you explain what you're trying to achieve a bit more? Since the prop types extend ButtonProps, as you mentioned, the loading state should already be supported. Isn't it?

Hey @robinmetral, yes it is supported and it works, but generates a typescript error because the isLoading and the loadingLabel props are not allowed (marked as never).

So my PR does not change any implementation data, just updates the types.

Copy link
Contributor

@robinmetral robinmetral left a comment

Choose a reason for hiding this comment

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

Ah my bad, somehow I misread this as removing the loading state 🤦 makes sense to me, let's :shipit:

.changeset/long-buttons-burn.md Outdated Show resolved Hide resolved
@robinmetral
Copy link
Contributor

@vascofg just tweaked the changeset a bit, I'll let you press the big green button and then handle the new version release 🙂

@vascofg vascofg merged commit bb9a8a3 into main May 17, 2022
@vascofg vascofg deleted the feat/is-loading-icon-button branch May 17, 2022 07:41
@connor-baer connor-baer mentioned this pull request May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants