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

Create Button with Spinner component #5271

Closed
techanvil opened this issue May 25, 2022 · 6 comments
Closed

Create Button with Spinner component #5271

techanvil opened this issue May 25, 2022 · 6 comments
Labels
P1 Medium priority Type: Feature New feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented May 25, 2022

Feature Description

Implement the Button with Spinner component.

The Button with Spinner component will look like a regular button, but with an activity spinner embedded within it.


Acceptance criteria

  • A component should be created which implements a button with an embedded spinner, as per the design.
    • Note: the design doc specifies creating a new component. If possible, updating the existing Button component to incude a loading prop that rendered the spinner would be a nice, reusable API. But this should only be done if straightforward in practice.
  • The visibility of the spinner should be controllable (via a prop).
  • The component should be present in Storybook.

Implementation Brief

  • Create a new SpinnerButton component in the assets/js/components folder. This component should the following:
    • Render the regular button component using the Spinner component in the trailingIcon property.
    • Proxy all props passed to the component down to the Button element.
    • Define a new state variable processing with false as the default value.
    • Define a new callback onClick that sets the processing state to true, calls the onClick function passed via props, and resets the processing value back to false after the passed onClick callback is executed.
      • Use the async/await approach to call the passed onClick callback to make sure that we reset the processing state only when the callback has finished working.
    • Set the processing variable to isSaving property of the Spinner element.
  • Add a corresponding storybook story for the new component.

Test Coverage

  • N/A

QA Brief

Changelog entry

  • Add a "button with spinner" component.
@techanvil techanvil added the Type: Feature New feature label May 25, 2022
@FlicHollis FlicHollis added the P1 Medium priority label May 27, 2022
@tofumatt tofumatt self-assigned this Jun 29, 2022
@tofumatt
Copy link
Collaborator

Should the ACs here require a new component? This strikes me as the sort of thing that would be good to add as a prop to the existing Button component.

@tofumatt tofumatt assigned techanvil and unassigned tofumatt Jun 30, 2022
@techanvil
Copy link
Collaborator Author

Hey @tofumatt, this is a good question and something I was pondering when writing the design doc. In the end I went with a separate component as I figure this is currently a one-off use case and it's nice to not overcomplicate the existing Button component.

In the design doc, which has obviously been signed off, I have described the new Button with Spinner component as follows:

This component should be buildable by combining the existing Button and Spinner components, either through composition or creating a new component which copies their internals.

So, I would be inclined to keep the AC on this basis. Or maybe something like this -

  • A component should be created which implements a button with an embedded spinner, as per the design.
    • Note, the design doc specifies creating a new component. However, the possibility of updating the existing Button component could also be examined.

What do you think?

@tofumatt
Copy link
Collaborator

I think that last phrasing is probably fine. My preference is to integrate it into the existing Button component because this would be a feature that would be useful for many buttons that cause a blocking/loading state. But if—in practice—that complicates the component too much then I'm fine if we don't.

@tofumatt tofumatt removed their assignment Aug 10, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 10, 2022
@eugene-manuilov
Copy link
Collaborator

  • The visibility of the spinner should be controllable (via a prop).

@techanvil do we really need to control the spinner via a property? I have added IB that explains how we can do it internally within the new component. Does it sound good to you?

@techanvil
Copy link
Collaborator Author

Hi @eugene-manuilov, that sounds fine to me and it would be compatible with the specced out usage of it in #5277, so I am happy to give this IB a :white_check_mark:.

I guess if we do find a need for a more declarative API then we can always add a loading prop in future which could override the internal processing state when set (or something along those lines). But we can cross that bridge if/when we come to it.

IB ✅

@techanvil techanvil removed their assignment Aug 11, 2022
@eugene-manuilov eugene-manuilov self-assigned this Aug 14, 2022
@eugene-manuilov eugene-manuilov removed their assignment Aug 16, 2022
@hussain-t hussain-t removed their assignment Aug 18, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Aug 18, 2022
@mohitwp mohitwp self-assigned this Aug 22, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Aug 22, 2022

QA Update ✅

@mohitwp mohitwp removed their assignment Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Feature New feature
Projects
None yet
Development

No branches or pull requests

7 participants