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

feat: optimize IconSpinner #3099

Merged
merged 4 commits into from
Sep 4, 2023
Merged

Conversation

fairlighteth
Copy link
Contributor

@fairlighteth fairlighteth commented Aug 31, 2023

Summary

  • Optimizes the IconSpinner module
  • Adds Cosmos fixtures
  • Create cosmos constant file with shared items.
Screen.Recording.2023-08-31.at.17.33.24.mov

@vercel
Copy link

vercel bot commented Aug 31, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
swap-dev ❌ Failed (Inspect) Sep 4, 2023 5:23pm

image,
size = 24,
children,
bgColor = '--cow-container-bg-01',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to get this value from a const or even better, make these color names an ENUM?

Imagine if we want to pass something else.
How will we know what values are expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imagine if we want to pass something else.
How will we know what values are expected?

I think there we probably need to agree to only always pass CSS color variables? Can you elaborate how an ENUM would add value here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The colors can be defined in a central place, while also being more legible.

For example:

enum ColorVariables = {
  CONTAINER_BG_01 = '--cow-container-bg-01'
}

type Props = {
  // ...
  bgColor: ColorVariables
}

function Component({
  // ...
  bgColor = CONTAINER_BG_01
}: Props) {
  // ...

Note: code is not tested, might not work or need tweaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@fairlighteth
Copy link
Contributor Author

Merging to keep this going. @alfetopito I've addressed your ENUM comment. Feel free to comment on this post-merge.

@fairlighteth fairlighteth merged commit 2781eb7 into permit-styles-modal-1 Sep 4, 2023
4 of 5 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 4, 2023
Comment on lines +67 to +69
enum ColorVariables {
CONTAINER_BG_01 = '--cow-container-bg-01',
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's what I meant!
Sort of 😅

This is the idea, but the suggestion is to put all of the variable (names?) in this enum.
Then you use the enum in the source css where they are declared, and when naming them individually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This change by itself is breaking the build.

@alfetopito alfetopito deleted the permit-styles-modal-2 branch September 5, 2023 09:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants