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] Add loading spinner to button to convey action #3255

Closed

Conversation

davidicus
Copy link
Contributor

Closes #3254

Adds a spinner to the button component to indicate an action is being performed

Changelog

New

  • add performingAction boolean prop to the button component which will render a spinner if true.
  • add styles for spinner to place in the right padding space of the button
  • added an extra button to the story to showcase the new feature

Changed

  • packages/components/src/components/button/_button.scss
  • packages/react/src/components/Button/Button-story.js
  • packages/react/src/components/Button/Button.js

Testing / Reviewing

check the story. You should see a button with a spinner

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for the-carbon-components ready!

Built with commit ee1d6fa

https://deploy-preview-3255--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for carbon-elements ready!

Built with commit ee1d6fa

https://deploy-preview-3255--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit ee1d6fa

https://deploy-preview-3255--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for the-carbon-components ready!

Built with commit 91cc7d1

https://deploy-preview-3255--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit 91cc7d1

https://deploy-preview-3255--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for carbon-elements ready!

Built with commit 91cc7d1

https://deploy-preview-3255--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for the-carbon-components ready!

Built with commit 5dbec21

https://deploy-preview-3255--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for carbon-components-react ready!

Built with commit 5dbec21

https://deploy-preview-3255--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Jul 2, 2019

Deploy preview for carbon-elements ready!

Built with commit 5dbec21

https://deploy-preview-3255--carbon-elements.netlify.com

@joshblack
Copy link
Contributor

cc @carbon-design-system/design if you all have a chance to review this pattern 👍

@joshblack joshblack requested a review from a team July 2, 2019 20:29
@ghost ghost requested review from abbeyhrt and vpicone and removed request for a team July 2, 2019 20:29
@aagonzales
Copy link
Member

Making sure I understand, is it adding this button variation in, that's the only new thing?
image

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Spacing should match the same spacing as "button with icon"
image

@davidicus
Copy link
Contributor Author

@aagonzales that is all this is adding. I will change the padding as suggested

@asudoh
Copy link
Contributor

asudoh commented Jul 2, 2019

@aagonzales Any thoughts on how this effort relates to our inline loading component? Specificially, wanted to see if this variant should contain the completed state, or not. I'm guessing not, but just wanted to double-check. Thanks!

@davidicus
Copy link
Contributor Author

@jeanservaas I have added a scenario to the original issue I had opened here. Please let me know if this is what you are looking for or if there is any other clarification I can provide.

@vpicone vpicone removed their request for review July 16, 2019 03:59
@jnm2377 jnm2377 requested a review from a team July 22, 2019 18:55
@ghost ghost requested review from designertyler and removed request for a team July 22, 2019 18:55
Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

On the dev side, it looks good to me. And based on the comments in the issue, it definitely seems like a use case we should support. Only thing that might change after design reviews is deciding if we'll stick with disabled colors for now or create new tokens for more of a "faded" primary button look. I'd prefer the latter, but I think it's up to design.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

I'd still like to see a version where the spinner code is not baked into <Button> - Which means, story-only change.

@jeanservaas
Copy link
Collaborator

jeanservaas commented Jul 23, 2019

  1. alignment looks OK in chrome but this is happening in Safari

image

  1. Also, is there a reason why you don't want to use the enabled spinner for the progress button per the thread in Add loading spinner to Button #3254 ?

image

  1. One other thing I noticed, it looks like you're using the large spinner in the button, the stroke is thinner... right @aagonzales

@davidicus
Copy link
Contributor Author

davidicus commented Jul 23, 2019

@jeanservaas Thanks for the feedback. This PR was all pre our discussion that you reference above so I have no issue in using an enabled spinner. I am more than happy to make these changes but it seemed like @asudoh may be against this addition to the button.

Either way, I am happy to update this PR however the Carbon team seems fit. I would love to have it baked in with the changes you mention here. Please advise.

@jeanservaas
Copy link
Collaborator

@davidicus

ah ok gotcha. Yeah, I would have to defer to @asudoh on the story only change.

@asudoh
Copy link
Contributor

asudoh commented Jul 23, 2019

@davidicus Thank you for understanding - Let's go with story-only (and possibly with some style) changes for now. If we find the usage story (in consumer devs perspective) is very difficult we can re-consider.

@stale
Copy link

stale bot commented Aug 22, 2019

We've marked this issue as stale because there hasn't been any activity for a couple of weeks. If there's no further activity on this issue in the next three days then we'll close it. You can keep the conversation going with just a short comment. Thanks for your contributions.

@stale stale bot added the status: inactive Will close if there's no further activity within a given time label Aug 22, 2019
@laurenmrice
Copy link
Member

not stale

@stale stale bot removed the status: inactive Will close if there's no further activity within a given time label Aug 23, 2019
@jnm2377
Copy link
Contributor

jnm2377 commented Aug 27, 2019

Hi @davidicus are you still working on this?

@davidicus
Copy link
Contributor Author

@jnm2377 I will make a change to the storybook. Just had some other priorities. WIll update soon!

@jnm2377
Copy link
Contributor

jnm2377 commented Sep 10, 2019

Hi @davidicus thanks so much for taking the time to contribute this example. 🙏 It seems that this PR has been stale for a while. We'll be closing it in a week if it's not updated and merged by then. If you don't have the bandwidth to continue work for this in the next week, we'd be happy to have you open a PR again once you've had the chance to work on it. 💯

@jnm2377 jnm2377 added status: inactive Will close if there's no further activity within a given time status: stale warning ⚠️ labels Sep 10, 2019
@stale stale bot removed the status: inactive Will close if there's no further activity within a given time label Sep 10, 2019
@jnm2377 jnm2377 closed this Sep 17, 2019
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.

Add loading spinner to Button
10 participants