site-kit-wp
site-kit-wp copied to clipboard
Create Button with Spinner component
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
loadingprop that rendered the spinner would be a nice, reusable API. But this should only be done if straightforward in practice.
- Note: the design doc specifies creating a new component. If possible, updating the existing Button component to incude a
- The visibility of the spinner should be controllable (via a prop).
- The component should be present in Storybook.
Implementation Brief
- Create a new
SpinnerButtoncomponent in theassets/js/componentsfolder. This component should the following:- Render the regular button component using the
Spinnercomponent in thetrailingIconproperty. - Proxy all props passed to the component down to the
Buttonelement. - Define a new state variable
processingwithfalseas the default value. - Define a new callback
onClickthat sets theprocessingstate totrue, calls theonClickfunction passed via props, and resets theprocessingvalue back tofalseafter the passedonClickcallback is executed.- Use the
async/awaitapproach to call the passedonClickcallback to make sure that we reset theprocessingstate only when the callback has finished working.
- Use the
- Set the
processingvariable toisSavingproperty of theSpinnerelement.
- Render the regular button component using the
- 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.
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.
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
Buttoncomponent could also be examined.
- Note, the design doc specifies creating a new component. However, the possibility of updating the existing
What do you think?
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.
- 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?
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:
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.