Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add spinner pattern and Spinner component #132

Merged
merged 3 commits into from
Jul 1, 2021
Merged

Add spinner pattern and Spinner component #132

merged 3 commits into from
Jul 1, 2021

Conversation

lyzadanger
Copy link
Contributor

@lyzadanger lyzadanger commented Jun 29, 2021

This PR adds a shared spinner pattern and corresponding Spinner component.

Motivation: The spinners are styled differently in our applications, and their usage within a single application is also inconsistent. In the immediate term, I'd like to use a loading spinner within a component on LMS, so this feels like the right time to drop this in.

This implementation is copied over from the client, with a little bit of simplification, and I made the Spinner component take a classes prop for better developer control.

This implementation is a variant of the lms Spinner, using an animated SVG.

To implement this, I had to update the sass dependency in this package. Needed the math.div function. The version here is in line with what the client and lms have.

The above is no longer the case, since the implementation is SVG-based, but there's no harm in bumping SASS.

This PR currently depends on #131 for my own convenience, but it doesn't strictly need to if that one gets mired.

Try it out

On this branch, run make dev and visit the Pattern Library's spinner pattern and Spinner components pages.

Screen shot (doesn't show animation):

image

@@ -0,0 +1,76 @@
// CSS Spinner modified from http://dabblet.com/gist/7615212
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much a direct c-n-p from the client.

@lyzadanger lyzadanger requested a review from robertknight June 29, 2021 21:49
Base automatically changed from rename-organisms to main June 29, 2021 22:21
Copy link
Contributor

@LMS007 LMS007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections to this PR other than deciding which is the right spinner to move forward with. LMS vs Client. The advantage of the SVG spinner is perhaps a whole lot less css, but it may be hard to tweak, slow down, etc.

*/
export function Spinner({ classes = '' }) {
return (
<span className={classnames('Hyp-Spinner', classes)}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently LMS uses an SVG. I don't have a strong preference for either, but I think the svg was added after the css spinner in the client. We could discuss tomorrow and pick one or othe other and then replace as we move forward.

@lyzadanger
Copy link
Contributor Author

@LMS007 Wow, I'm so glad you pointed the SVG implementation out from LMS! That is dead simple implementation; let me poke around and see if it can work for our needs. I tend to find the client one more visually attractive—that's why I used it here—but it was daft of me to not at least take a look at the lms variant. The implementation of that one is so simple that it may outweigh the slightly more attractive (but super complex) client version. I'm going to pop this PR into draft while I look into it.

@lyzadanger lyzadanger marked this pull request as draft June 30, 2021 11:26
Need the recently-added `math.div` function for `spinner` styling.
@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #132 (06bad8e) into main (6297d4d) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #132   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        11    +1     
  Lines          134       138    +4     
  Branches        41        42    +1     
=========================================
+ Hits           134       138    +4     
Impacted Files Coverage Δ
src/components/Spinner.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6297d4d...06bad8e. Read the comment docs.

@lyzadanger lyzadanger marked this pull request as ready for review June 30, 2021 11:58
@lyzadanger
Copy link
Contributor Author

lyzadanger commented Jun 30, 2021

OK! Reboot! Thank you @LMS007 for the heads up; I've opted for the much simpler LMS variant here, which uses an animated SVG. While I'm still not 100% in love with the exact visual styling of the animated SVG per our own design style (p.s. I did take a look at other options in the originating SVG repo and this seemed to be one of the best alternatives). There's nothing wrong with the SVG! I just think the visual style of the (complexly-styled) version in the client is a little more in line with our design language.

The good news is that it's dead simple to drop in a different SVG in future if we find or make one that fits our style better! Another argument for centralizing this component...

For developer ergonomics, I added, in this latest implementation, a size property, as the loading spinner is often displayed large, in relation to the local font-size.

@lyzadanger lyzadanger requested a review from LMS007 June 30, 2021 12:04
@@ -0,0 +1,36 @@
<!--
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied from LMS

Copy link
Contributor

@LMS007 LMS007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG! I think the SVG is just a bit more simple so that may be worth it alone.

The pattern lib is really getting flushed out, nice to see it grow.

@LMS007
Copy link
Contributor

LMS007 commented Jun 30, 2021

er ergonomics, I added, in this latest implementation

yep, I think the size prop is useful, thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants