site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Create Button with Spinner component

Open techanvil opened this issue 3 years ago • 6 comments

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.

  • Design Doc: https://docs.google.com/document/d/1DeWo38lcV7lvLFfcmt-mQ0iaxzB4qfiPArs_yZzYkIM/edit#heading=h.vpcddsd4qdav
  • Figma: https://www.figma.com/file/vMaCWwr6lpk4PrJWb7jIpz/GA4-Banner-Input?node-id=1082%3A1550

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

  • Go to this storybook story: https://google.github.io/site-kit-wp/storybook/develop/?path=/story/components-spinnerbutton--default-button
  • Verify that the spinner button shows the spinner for 5 seconds when you click on it.

Changelog entry

  • Add a "button with spinner" component.

techanvil avatar May 25 '22 17:05 techanvil

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 avatar Jun 30 '22 15:06 tofumatt

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?

techanvil avatar Jul 06 '22 09:07 techanvil

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 avatar Aug 10 '22 15:08 tofumatt

  • 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?

eugene-manuilov avatar Aug 10 '22 17:08 eugene-manuilov

Hi @eugene-manuilov, that sounds fine to me and it would be compatible with the specced out usage of it in https://github.com/google/site-kit-wp/issues/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 :white_check_mark:

techanvil avatar Aug 11 '22 15:08 techanvil

QA Update ✅

  • Verified storybook story : https://google.github.io/site-kit-wp/storybook/develop/?path=/story/components-spinnerbutton--default-button
  • The spinner button showing the spinner for 5 seconds when we click on it.

mohitwp avatar Aug 22 '22 10:08 mohitwp