Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Add loading spinner #75

Merged
merged 10 commits into from
Aug 23, 2019
Merged

Add loading spinner #75

merged 10 commits into from
Aug 23, 2019

Conversation

jgzuke
Copy link
Contributor

@jgzuke jgzuke commented Aug 21, 2019

Designs at https://app.zeplin.io/project/5c7dcb5ab4e654bca8cde54d/screen/5d56d40acf2df541b112fc57

Spinner code mostly from https://codepen.io/daniman-the-bold/pen/jgjqBw, thanks @daniman 🎉

Also made the DemoGroup shared since it was being used in TextField, Button, and now Loader stories.

@jgzuke jgzuke added the minor Increment the minor version when merged label Aug 21, 2019
Copy link
Contributor

@justinanastos justinanastos left a comment

Choose a reason for hiding this comment

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

Left some questions and suggestions. Reach out to me with any questions!

src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/Loaders/LoadingSpinner.tsx Show resolved Hide resolved
src/Loaders/Loaders.story.tsx Show resolved Hide resolved
src/Loaders/Loaders.story.tsx Outdated Show resolved Hide resolved
src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
@justinanastos
Copy link
Contributor

This also needs some input from @adamatorres about how to handle the disabled button state where we can't see the orbit

@daniman
Copy link
Member

daniman commented Aug 21, 2019

This is pretty great Jason, thanks for putting my pen into code here! Two quick comments on the styles:

#1 –– I think the orbit line and orbit circle probably need a bit of tweaking at the different sizes. I'm not sure what the dimension are myself.

#2 –– I think Tim had a different timing function for the orbit when it's in a button. Looks like it's a slightly faster linear animation, not a cubic bezier. You should sync up w him and Adam on the intent there: https://meteor.slack.com/archives/CFSCN3G7L/p1565744964008200.

src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/Loaders/Loaders.story.tsx Show resolved Hide resolved
Copy link
Contributor

@justinanastos justinanastos left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes! I'm leaving a comment on a resolved comment from before so I want to make sure you see this:

I didn't say this in the comment so there's no fault on your part; I was hoping you'd respond to this message instead of just making the change and requesting a new review.

It's important to me that you understand why I'm requesting this change and why I think this makes future code changes safer. Why? If you are looking for the same things, then you'll catch places where I might have missed strong type safety in my PRs. A rising tide lifts all ships!

Never be afraid of communicating too much!

src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/Loaders/Loaders.story.tsx Outdated Show resolved Hide resolved
src/Loaders/LoadingSpinner.tsx Show resolved Hide resolved
src/Loaders/LoadingSpinner.tsx Outdated Show resolved Hide resolved
@justinanastos
Copy link
Contributor

Get eslint to pass (the error is auto-fixable, looks like it's time to add a global vs code extension that auto fixes things).

Copy link
Contributor

@justinanastos justinanastos left a comment

Choose a reason for hiding this comment

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

Get eslint to pass and merge away!

@jgzuke jgzuke merged commit b40cfef into master Aug 23, 2019
@jgzuke jgzuke deleted the jgzuke/add-loading-spinner branch August 23, 2019 20:18
@justinanastos
Copy link
Contributor

🚀 PR was released in v1.8.0 🚀

@justinanastos justinanastos added the released This issue/pull request has been released. label Aug 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants