feat(Spinner): Adding Spinner component - #100

PR #💯 🎉 🎊

@waldenraines haha 👍 good catch! Congrats to the patternfly-react community!

haha thanks for the enthusiasm @waldenraines 👍 .

@danseethaler @jgiardino I don't see much else on this other than potentially providing an additional example with the dark background (see patternfly.org example) and the test page example. You can achieve the styles there giving a black background and applying the spinner-inverse class.

As far as the stateless component goes... 👍

What else here @jgiardino ?

Good call in the inverse option @priley86. I've updated to include that.

As a side thought I'm wondering if we want to add more of the simple components into the widgets grouping on the sidebar. Maybe the badges, popover, and tooltips. Any thoughts on that?

Thanks for adding this!

The only questions I have are regarding the inline implementation…

There isn’t much documentation about the use case for the inline spinner, but my expectation is that the text that displays next to an inline spinner is related to the loading process. And when loading is complete, then the text is no longer relevant and should be removed along with the spinner.

However in this implementation, it looks like the text that displays for the inline spinner remains after loading is complete.

@danseethaler - Does the current implementation of this component support the use case I just described?
@mcarrano, @LHinson, @Rohoover - Are my assumptions about this use case correct?

Additionally, the test page includes examples of the inline spinner where it is wrapped by different html elements: h1, h2, h3, p
Is it possible to use these elements with the spinner component?

@mcarrano @LHinson What are your thoughts about @danseethaler suggestions for the storybook organization? I've lost touch with the information architecture work that's currently happening. Will we be getting recommendations from that work on how to organize the components in the Storybook?

Really good points @jgiardino. The current implementation does allow you to wrap the spinner with any component (h1, p, etc.) just like the inline examples on the test page. In the case when the inline spinner is wrapped with an h1 or other tag it will be up to the application to remove that when loading completes. It's not removed on the story since it would be just a blank screen.

The primary idea with this component was to provide a wrapper around components that you don't want to show until loading is complete. This is common in React when a parent component needs to load some data before it renders it's children.

@jgiardino @mcarrano @LHinson Storybook 3.3 will be coming soon and bring a wealth of other benefits too. My vote is to start introducing this kind of structure eventually (but at the moment, I don't think it's ready).

• Storybook Hierarchies: 1699 Have Stories on each level of hierarchy storybookjs/storybook#1763

@jgiardino your assumption is correct, as in most cases I would expect that the text associated with the spinner would be some type of 'loading in progress' message'.

Regarding the Storybook organization, the information architecture is still a work in progress, but I will say that we are definitely moving away from the idea of using "widgets" as a category. It's too unclear what's a widget and what's not. In general our testing has found that a flatter organization is preferred, with some exceptions. We will definitely want to better sync the storybook sites and the patternfly.org site down the road. But it's premature to do that at this point.

@mcarrano thanks! Let us know what you find. Just wanted to share that as an option coming soon...

Any other thoughts or reservations before merging?

@danseethaler I don't have any reservations about this from a JS standpoint (it seems this is more just Storybook examples/PatternFly demonstration specifics). I think @jgiardino's suggestion about showcasing the h1,h2,h3,p examples is useful for design demonstration as our existing test page looks like this:
https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/spinner.html

@priley86 oh yeah I wasn't sure if @jgiardino was recommending we show those in the storybook since they're already on the test page. I'm happy to add them though, either way.

Regarding the h1, h2, h3, p examples, I just wanted to note if these cases were supported in this component or not, so that we can track the status in the issue. I'm not opposed to showing them in the storybook, but I wouldn't say that's necessary for merging this.

The only concern from me is the difference between inline being true vs. false, in that it seems like the implementation of this component in a product would be different depending on whether I was displaying inline text or not. But I'm happy to defer to everyone else on whether this is even an issue—it was just something I noticed looking the storybook. :-)

Hi @jgiardino , in the "Inline" section of test page, I find that the size of spinner has nothing to do with h1 tag. Under the hood, it's still specified by className "spinner-lg".

<h1>
  <div class="spinner spinner-lg spinner-inline"></div>
        Inside an &lt;h1&gt;
</h1>

As a result, I don't think it's necessary to add new demos to spinner storybook according to the h1, h2, h3, p examples.

Yeah I think we're good here 👍

The difference in the storybook implementation for inline is just due to the difference usage. The HTML structure needs to be different for inline vs wrapper so the usage is a little different. Thanks for the feedback everyone!

Thanks for the clarification, @danseethaler!

Let's go ahead and merge this one (it seems the behavior is as expected). @jgiardino feel free to circle back on the Storybook stories later if you'd like to make any updates there.