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

Rework colors variables to use primary/secondary and make them configurable #146

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

Conversation

alimtunc
Copy link
Collaborator

@alimtunc alimtunc commented Sep 30, 2022

I've added the simplest way I found to have custom CSS color on lemverse, but firstly I've replaced some SCSS color variables to be generic using primary and secondary keyword.

Now, we need to just need to update primary and secondary color on settings.json

.gitignore Outdated
@@ -108,3 +108,6 @@ dist

# TernJS port file
.tern-port

# scss
core/client/_theme.scss
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with this solution is that the project crashes on the import because, by default, the
_theme.scss file doesn't exist, I'm looking for a solution with an optional import system

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, don't worry. @ramnes suggested me to use something like this document.querySelector(':root').style.setProperty('--main-color', Meteor.settings.public.mainColor) to update it in the first runtime, with this, we won't need to have a separate file.

I will update my PR soon

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, it's a good idea!
I looked on my side in SCSS and it's complicated, there are optional imports but it only exists in LESS 😕

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, looks like it's more complicated than I thought.. we can't combine var(--primary-color) with the darken() function of SCSS https://stackoverflow.com/questions/64595291/is-there-a-way-to-use-sass-lighten-and-darken-with-css-variables

The only way to use CSS variables, would be to use a similar function like this https://stackoverflow.com/a/55330103 ... And actually we use darken() many times. Same as ligther().

Copy link
Collaborator Author

@alimtunc alimtunc Oct 3, 2022

Choose a reason for hiding this comment

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

Ok forget the hsl(), we can't use it too. To have the same color from #hex to hsl(), we will have a hsl function like this :

Capture d’écran 2022-10-03 à 16 31 29

And the pain is, that we can't play easily with the darkness or lightness color. We should fine another solution for this. I let you check this to understand how light and dark work with hsl https://stackoverflow.com/questions/44703550/sass-color-darken-with-hsl-color-codes

@alimtunc alimtunc marked this pull request as draft October 3, 2022 09:05
@ramnes ramnes added the enhancement New feature or request label Oct 3, 2022
@alimtunc alimtunc marked this pull request as ready for review October 4, 2022 13:06
@alimtunc alimtunc force-pushed the configurableTheme branch 2 times, most recently from 1fe4d66 to a4a1cfb Compare October 4, 2022 13:09
@alimtunc
Copy link
Collaborator Author

alimtunc commented Oct 4, 2022

Okay, all good for a review, color customization is now on settings.json 🙌 @Donorhan

@alimtunc alimtunc force-pushed the configurableTheme branch 3 times, most recently from 172b75d to 2d4746e Compare October 7, 2022 16:36
@xsyann xsyann force-pushed the configurableTheme branch 3 times, most recently from 868c52a to 26db8ea Compare October 17, 2022 07:26
@ramnes
Copy link
Collaborator

ramnes commented Oct 25, 2022

@Donorhan Is there anything we can do? :)

@Donorhan
Copy link
Contributor

I'm not entirely comfortable with this integration. Going through JS and CSS variables seems a bit overkill to me and at the same time I haven't found any other solutions yet 😅

I did think about having a file that overloads the variables file at the build but the Meteor build seems a bit limited for that.

@ramnes
Copy link
Collaborator

ramnes commented Oct 28, 2022

Why does it seem overkill to you? That's how we achieved #158 as well so both PR are quite consistent.

The settings are read at runtime so if we want to keep all the configuration stuff there, we have to work at runtime as well and there isn't many ways.

Also, settings at runtime could end up being super useful if we want to make the UI theme editable on the level side later. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants