-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Use styled-components in Loader #2980
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
Use styled-components in Loader #2980
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good at first glance. I have some thoughts but they are not necessarily problems. Just thoughts. You don't need to change anything 😄
I'm marking this as "Approved" and I just need to run your branch and verify that it looks correct before merging.
In general I think the we should not use &&&
unless it's actually necessary because it makes it harder to override the styles for specific use cases which might want to extend the style. In reality it does not make a difference for the Loader
because we use the same <Loader/>
everywhere and do not ever override any of these styles (like "use the loader but make it a different color").
There are multiple ways to handle setting the animation-delay: -1.0s;
only on the second circle. The &:nth-child(2)
which you have here is definitely one of those ways and it's fine. ✔️ The potential downside is that it makes it so that LoaderCircle
is not reusable as its own component since it renders differently depending on its context, but that is moot because we only use the LoaderCircle
here as part of the Loader
and I can't think of any other use for it. We don't even export it which is correct. Other ways would be: 1. to pass delay
as a prop to the LoaderCircle
component, like <LoaderCircle delay={0}/><LoaderCircle delay={-1} />
. 2. To wrap the core LoaderCircle
for the second circle like const DelayedLoaderCircle = styled(LoaderCircle){ animation-delay: -1s; }
and have <LoaderCircle/><DelayedLoaderCircle/>
. But both of those seem really overcomplicated so I think that what you have is the best option.
I would like to work on this issue |
Progress on #1760
Changes:
Looks identical to before on Windows Chrome.
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123