-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Ability to add a custom theme dark and light colors #44
base: main
Are you sure you want to change the base?
Conversation
You have to wrap all the color names in quotes |
I was getting the CSS errors too, because the color names, some of them conflict with CSS colors, so it breaks the logic. So wrapping them in double quotes seems to fix it on my end. |
@odama626
What colors do you mean? Every color I use from |
I had to add quotes to $colors at the bottom of colors/_index.scss according to my package.json "sass": "^1.83.4", |
In your terminal, run:
and you'll see the version installed on your system, that's going to be the version you're using when its building. I took out the |
pnpm exec sass --version 1.83.4 compiled with dart2js 3.6.1 |
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.
oh I think I see what the problem is,
@use "../../settings" as * ;
means you have two different definitions of $color-mappings in theme-colors
so the theme code I wrote errors on theme-colors.$color-mappings since there are multiple definitions, and it doesn't make sense.
I was able to fix the error locally with the following changes.
I didn't realize what you were trying to do until I reread it this evening, being able to define custom themes is pretty cool, I achieved something similar in my project by overriding --pico-primary variables
@@ -3,487 +3,490 @@ | |||
@use "../../colors" as *; | |||
@use "../../settings" as *; |
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.
@use "../../settings" as *; | |
@use "../../settings" |
), | ||
), | ||
$color-mappings |
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.
$color-mappings | |
settings.$color-mappings |
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.
Would we need to use the settings.$theme-color
in that get()
function as well?
Since @use
started rolling out, I've always used the @use 'file' as *;
way to get the variables. I searched around briefly to see the benefits of both, and I can see why we could use file.$variable
but haven't seen anything noting whats the proper way to do that.
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.
Yeah I think so
looks like I missed a change @function get($key, $mode: "light") {
- @return map.get(map.get(map.get($color-mappings, $theme-color), $mode), $key);
+ @return map.get(map.get(map.get($color-mappings, settings.$theme-color), $mode), $key);
} |
Ahh you commented that before I submitted my comment I wrote out earlier hahah. Have you found an article or somewhere in the docs that says to get the variables that way vrs the |
no, I haven't read anything on it, I just realized that sass could use some better error messages lol |
one thing that I did notice that could be kind of confusing as an end user of the library -- if you set a $theme-color that isn't in the theme-mappings (I forgot to set $custom-theme-mappings for instance) it throws a $map of null doesn't exist error confusing but -- I couldn't find any try / catch support in sass, so I guess we can't make an easier to understand error message for it |
is there anything I can do to help with this? |
I'll see about getting this added to the 2.3.* branch |
I only had to change 2 files to make this possible, and would like to release it, but I'm worried about breaking changes with the _theme-colors.scss file because I have @odama626 change from #28 in my build script which threw errors on
theme-colors.$color-mappings
not found.My Client liked the colors and themes provided within Pico, but wanted the overall color to be darker.
I did not want to edit the node_modules folder, and I didn't want to edit the css --primary-* colors in my CSS, so I found a way to inject our own color theme into the package to compile.
Steps to test in your project
Within your project where you already have installed PicoCSS with
npm -i @yohns/picocss
you have your custom SCSS file where you can turn off or on the modules and other settings, here's what I did.Update the 2 files
Update the 2 scss files to add the changes in this pull request's scss directory, its 2 files.
Create your theme
Create a
_custom_theme.scss
file, mine is located inassets/scss/custom/_custom_theme.scss
with your custom, name, dark and light colors:Update your index.scss`
And then in your
index.scss
file where you compile your own version you can have the following,Mine is located:
assets/scss/index.scss
Build it
And when you compile your sass, it will use your custom color theme instead of one of the presets provided.
If you don't have the custom build in your project with PicoCSS, and would like it let me know and I'll share. Its basically the same build that is in the main build here without building all the themes.
Let me know what you think, any issues, or have questions!