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

fix: light-name-color -> color-name-light; added light variants if mi… #2750

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CheariX
Copy link
Contributor

@CheariX CheariX commented Oct 5, 2024

I found that some color names used a different naming scheme than other.

E.g. $light-cyan-color should be $cyan-color-light according to the other light and dark variants.
Also, I added light variants in case they were missing.

@george-gca
Copy link
Collaborator

george-gca commented Oct 5, 2024

I don't think they are exactly light variants, some of them might be just a light cyan instead of a cyan for light mode, for example.

Also if you changed the color name check if it isn't used anywhere. If it is, you should also change it there. If it isn't, why does it even exist? So maybe we should delete it.

There will be one day that one shall look at all our sass code and clean/organize it.

@CheariX
Copy link
Contributor Author

CheariX commented Oct 6, 2024

I don't think they are exactly light variants, some of them might be just a light cyan instead of a cyan for light mode, for example.

Oh, this is good to know. I did not know that the light variants are supposed to be cyan for light mode instead of light cyan. Thank you.

So this particular light-cyan-color is correctly named and used in a dark theme? Then we would need a light-cyan-color-light for the light theme?
Also, there are color names like green-color-bright, which sound to me like what this cyan should have been defined.

These namings are really confusing. 😃

Also if you changed the color name check if it isn't used anywhere. If it is, you should also change it there. If it isn't, why does it even exist? So maybe we should delete it.

I checked it before creating this PR but I could not find any references.

There will be one day that one shall look at all our sass code and clean/organize it.

😀

TBH, I don't care too much about this particular PR. It was just something that I stumbled upon during my last master merge.
If you don't like it, feel free to close/reject it.

@george-gca
Copy link
Collaborator

Are these new colors used somewhere? If not, I don't think we should create them, even though they will be discarded by PurgeCSS.

@alshedivat alshedivat changed the base branch from master to main October 20, 2024 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants