Add the required css to use Spinner in react-components

[solazio]

Currently the Spinner component in react-components is using some custom css. Would be good to add the styling to vanilla so we can remove the custom styling in react-components.

[sowasred2012]

@huwshimi @squidsoup we just took a look at this issue and we'd like your insight, because we're confused - looking at the storybook at least, it seems like the only styles here that affect anything are the padding in the base version, and the margin in the --inline variant. That line height (1.5rem) is inherited either from a parent button element, or the global html styles, and the span element is inline by default, so width: 100%; doesn't do anything, and the --inline variant seems unnecessary (aside from the additional margin).

It'd be good to discuss how this component is being used, and in what contexts these styles are needed to see if there's a way we can achieve the same result with what's already in Vanilla, or decide which of these styles need to be upstreamed.

[bartaz]

@huwshimi @squidsoup To add to previous comment - can you provide some screenshots of where the component is used in its different versions (default, inline, with text label, without)? So we can better understand what exactly is needed in terms of styling?

It seems it adds some padding around, tries to have inline variant (but is inline by default because of use of span).

Also, @anthonydillon suggests that possibly it should be called "Loader" not "Spinner" - I see there is "Loader" React component that was renamed to Spinner. What's the reason there?

[sowasred2012]

Looping in @Caleb-Ellis too - @Caleb-Ellis we were looking to get a bit of insight on this component, wondered if you could help? ^

[Caleb-Ellis]

Hey @bartaz, @sowasred2012!

I don't think this component needs any custom css at all. I'm fairly certain the only permutations that we use in MAAS are:

• Just a spinner icon
• A spinner icon with some text to the right ("Loading...")
• A spinner icon with some margin to the left ("inline")
• A spinner icon with both text and margin

I can see that there is some css for when a spinner is in a button, but we don't use that at all, and instead use the ActionButton component which keeps the button width consistent. I'm also not entirely sure why this component includes padding, and in fact in a few places I've used this component I've included u-no-padding to get rid of it. @huwshimi do you remember why there's padding?

As for the rename, I think @hatched made the case that because it's essentially just the Vanilla spinner icon with a couple of props it should be called "Spinner" instead of "Loader" (which it was called originally). I think "Loader" also implies that it might load something, which it doesn't.

[huwshimi]

I can see that there is some css for when a spinner is in a button, but we don't use that at all, and instead use the ActionButton component which keeps the button width consistent. I'm also not entirely sure why this component includes padding, and in fact in a few places I've used this component I've included u-no-padding to get rid of it. @huwshimi do you remember why there's padding?

Maybe the padding is a leftover from before ActionButton was a component.

[bartaz]

Thanks for clarifications @Caleb-Ellis @huwshimi.

So, are you saying that there is no need to this to become pattern in Vanilla ATM? Is it ok to just remove any styling from it?

It seems it still may need a way to make spacing between the icon and text correct, or to provide the margin in 'inline' version. If these styles are needed, we would need to provide them from Vanilla.

[anthonydillon]

Could we use the inline icon class or button.has-icon pattern to cover the to use cases?

The reason the name "Spinner" is a bad one is that it describes the style of the pattern. What if we changed the pattern to use a pulsing animation. I see the issue with "Loader" in the context to applications which load data although better then "Spinner".

Material design: "ProgressIndicator" Carbon: "Loading" Shopify: "Loading" Apple: "Loading"

[sowasred2012]

Happy new year @huwshimi @Caleb-Ellis!

Resurfacing this thread to get some closure - from the comments above, it looks like changes need to be made in react-components (the removal of custom styles), rather than Vanilla. Do you agree? If so, I'll close this issue and create one there instead; if not, let us know what styles you need and we'll get it into Vanilla.

[Caleb-Ellis]

Hey @sowasred2012, sorry seemed to have missed the ping! Yeah I think this isn't a vanilla-framework issue but really a react-components issue to just remove the custom scss and replace with what's provided by vanilla.