Spinner component

[avelinet]

Status: Ready to Merge

Reviewers: @jeffkamo @cole-sanderson @mlworks @nastiatikk @kpeatt @ry5n JIRA: CSOPS-1106

[ry5n]

Some thoughts:

• I imagine box-shadow performance would be OK because the rendered area is small?
• Would the CSS be any more compact/easier to understand by using actual elements rather than box-shadow?
• Looks like the $spinner__spacer variables are setting the x position of the individual dots? And they are kind of private vars… should we declare this kind of variable with the same naming convention as public configurable vars?

Looking good though. Awesome to see tests and everything in place!

[avelinet]

@ry5n The performance seems fine on device. So long as you don't set opacity 0 and have it going in perpetuity ;)

Yes the animation is simpler by using elements, however I like how clean the html is with the 1 element. I've tried both approaches, and I prefer working with the 1 element approach in the project itself.

A different naming convention for private vars sounds good, did you have something in mind?

[ry5n]

@avelinet Rationale seems solid on performance and markup. Nice that it wouldn’t need a template in builds. +1.

I have some very tentative thoughts about private vars. Would be ideal if they did not pollute the global variable space as they normally do when defined at the root level. AFAIK the only way to get that is to wrap the styles (but not the public variables) in basically a null block, like @if true { /* private vars */ /* styles */ }. Seems gross, but maybe if it’s declared with some kind of mixin that made sense to everyone, it could work.

I’m not sure about any of that, but if it worked out, we shouldn’t need a new naming convention at, which would be a plus.

[jeffkamo]

Couldn't we solve the private variable issue by declaring the variables inside the component blocks?

Example...

.c-component {
    $component__size: 16px;

    font-size: $component__size;
}

.c-elsewhere {
    font-size: $component__size;
}

Working example here, notice that the variable is undefined in the c-elsewhere block.

[jeffkamo]

The only downside is that the variables would be declared across the stylesheet. Doesn't sound so bad...

[ry5n]

Only if we nest everything inside .c-component, which would mean generating selectors like .c-component .c-component__child. Not something we should do IMO.

[kpeatt]

@alyseadlard just shared this with me: https://github.com/tobiasahlin/SpinKit – maybe we can crib some stuff from it?

[avelinet]

@kpeatt see https://github.com/mobify/stencil/tree/component-loader/dist/components/loader

[kpeatt]

Ah, thanks @avelinet!

[ry5n]

Sorry to leave this for so long. It’s solid. Just a couple of things, picking up from where we left off:

• Since the spinner__spacer variables are for internal use only, let’s call them $spacer, $spacer2, etc. for now and put them under separate heading like “Private variables”.
• I think smaller or larger spinners should probably be left up to the developer, so maybe we don’t need the c--scale variation off the bat? I think we can merge this either way.
• In the test’s index.html, the body needs class="c-fixture-body" otherwise it’s full-width.

+1, let’s get this in!

[avelinet]

Thanks for the feedback @ry5n, I've made the updates.

[jeffkamo]

+1, awesome work @avelinet!