Skip to content

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

Merged
merged 7 commits into from
Aug 2, 2024

Conversation

adityagarg06
Copy link
Contributor

@adityagarg06 adityagarg06 commented Feb 1, 2024

Progress on #1760

Changes:

  • Migrate styles from _loader.scss into individual styled components in the Loader.jsx file.
  • Deleted the _loader.scss.

Looks identical to before on Windows Chrome.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

lindapaiste
lindapaiste previously approved these changes Feb 1, 2024
Copy link
Collaborator

@lindapaiste lindapaiste left a 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.

@raclim raclim added Area:CSS For styling or layout issues handled with CSS/SASS Type:Task Tasks tied specifically to developer operations and maintenance labels Feb 6, 2024
@raclim raclim merged commit 736cc60 into processing:develop Aug 2, 2024
1 check passed
@Noorain464
Copy link

I would like to work on this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:CSS For styling or layout issues handled with CSS/SASS Type:Task Tasks tied specifically to developer operations and maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants