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

Turn off syncAnimationRestarts for chromatic and buttons #141

Merged
merged 4 commits into from
Nov 22, 2019

Conversation

jgzuke
Copy link
Contributor

@jgzuke jgzuke commented Nov 19, 2019

Add syncAnimationRestarts (default true) prop to LoadingSpinner to replace usage of isChromatic. Also set to false in stories and buttons. We only really need it false in stories but this seemed easier than passing the syncAnimationRestarts prop to buttons as well, it also seemed like it would look more natural if button loaders weren't synced.

Im a bit conflicted about whether this should default to false instead. I feel like the default makes more sense to be false since we only really want this for our full page loaders, however it seems likely for people to miss setting the prop. This is mitigated a bit if we only use the engine wrappers instead of this component directly.

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.

I can't think of a better way to solve this 🤷‍♂ ; left one question. Answer and I'll re-review 😃

src/Loaders/LoadingSpinner.stories.mdx Outdated 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.

This is failing CI. Please take a look and toss it back for review

@justinanastos
Copy link
Contributor

You also forgot to add a label.

@jgzuke jgzuke added the minor Increment the minor version when merged label Nov 20, 2019
@jgzuke jgzuke force-pushed the jgzuke/fix-chromatic-dependency branch from e097c2e to fb390f7 Compare November 20, 2019 21:26
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 this @jgzuke ! I left one minor to add a comment, then merge away!

}

export const LoadingSpinner: React.FC<Props> = ({
theme = "light",
size = "medium",
syncAnimationRestarts = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I'm sorry I keep doing this, but please add a jsdoc comment explaining what this does so we can see that from AGM. After that, merge away!

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.

This is going to cause issues for Chromatic tests in libraries consuming this package. I'm struggling to think of a solid solution to make this safe for consuming libraries while also not including chromatic in the tests. I am also running into the same issue for disabling animations in Modals.

Here's a hacky thought: what if we added a static property to the component itself that would disable the animation? That way we can do something like LoadingSpinner.animationSync = false in the storybook/chromatic setup and then all loaders would behave as we expect for visual testing.

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.

This is going to cause issues for Chromatic tests in libraries consuming this package. I'm struggling to think of a solid solution to make this safe for consuming libraries while also not including chromatic in the tests. I am also running into the same issue for disabling animations in Modals.

Here's a hacky thought: what if we added a static property to the component itself that would disable the animation? That way we can do something like LoadingSpinner.animationSync = false in the storybook/chromatic setup and then all loaders would behave as we expect for visual testing.

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 working through this with me @jgzuke !

@jgzuke jgzuke merged commit 62ee4d1 into master Nov 22, 2019
@jgzuke jgzuke deleted the jgzuke/fix-chromatic-dependency branch November 22, 2019 16:38
@apollo-bot2
Copy link
Collaborator

🚀 PR was released in v2.19.0 🚀

@apollo-bot2 apollo-bot2 added the released This issue/pull request has been released. label Nov 22, 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.

3 participants