-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Intergrate tiddlywiki palette colors and settings to custom CSS properties #9333
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
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for tiddlywiki-previews ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Confirmed: Leilei332 has already signed the Contributor License Agreement (see contributing.md) |
📊 Build Size Comparison:
|
Branch | Size |
---|---|
Base (master) | 2421.1 KB |
PR | 2433.4 KB |
Diff: ⬆️ Increase: +12.3 KB
/* Tiddlywiki's CSS properties */ | ||
|
||
:root { | ||
--tc-alert-background: <<colour alert-background>>; |
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.
The tc-
prefix is used for TW class names. So IMO it will be confusing if we use it for CSS variables too.
tv-
is used for TW variables. So imo --tv-
can be used for CSS variables. This would be more consistent
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.
Actually c
stands for color here. In custom properties there is no classes, but there exists color data type.
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.
Hi @Leilei332 I do think that using tc-
is confusing, and we need to adopt a different prefix here. Many appropriate prefixes have already been used (eg tv-
which is used for TW system variables).
Perhaps we might introduce a new v-
prefix for CSS variable, so you’d have v-alert-background
etc.
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.
Hi @Leilei332 I do think that using
tc-
is confusing, and we need to adopt a different prefix here. Many appropriate prefixes have already been used (egtv-
which is used for TW system variables).Perhaps we might introduce a new
v-
prefix for CSS variable, so you’d havev-alert-background
etc.
If we don't care about the potential name conflict between CSS settings and palette colors, I think tp-
prefix is acceptable. If we do care, we may use tp-c-
prefix.
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.
I think --cc-
for CSS color and
--cp-
for CSS parameter would be OK and consistent.
It avoids name clashes with user CSS variables, so they do not use any prefix. It's also easy to discuss at Talk. Users may use a shortcut for cc-variables
or cp-variables
-- It's not correct, but we still will know what is meant.
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.
I think
--cc-
for CSS color and
--cp-
for CSS parameter would be OK and consistent.It avoids name clashes with user CSS variables, so they do not use any prefix. It's also easy to discuss at Talk. Users may use a shortcut for
cc-variables
orcp-variables
-- It's not correct, but we still will know what is meant.
This isn't acceptable. It can't show that this custom property is only for tiddlywiki.
This PR adds a stylesheet to intergrate palette colors and theme settings to CSS custom properties
Compared to using wikitext syntax, using custom CSS properties has these benefits:
var()
is more advanced, it supports fallback.TODO