Migrate to bulma-css-variables and introduce dark mode #954
dpschen
konrad
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
4 Participants
No due date set.
Blocks
#1023 feature/bulma-css-variables-reduce-import-size
vikunja/frontend
Reference: vikunja/frontend#954
Loading…
Reference in New Issue
No description provided.
Delete Branch "adrinux/frontend:bulma-css-variables"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Currently uses CSS prefers-color-scheme: dark and so follows the OS level dark preference. This will need to be made user configurable in the Vikunja UI as discussed in Issue #756
For now this allows for preview, discussion, bug hunting and refinement of dark mode colours.
Two open bugs: https://kolaente.dev/adrinux/frontend/issues
Changes:
Dark mode notes
By 'crude dark mode' I mean
So feedback please on whether bulma-css-variables is the way forward and if so whether there are any other bugs in dark mode.
Closes #756
@ -3,10 +3,10 @@@import "variables";The
@import "colors";should be removed here and instead added inglobal.scssbelow the bulma imports.Probably also below the
themeandcomponentsoverwrites, but I'm not 100% sure about that.Reason: the
colors.scssproduces now CSS output (the:rootrules) — it was compiled to nothing before.Since it's prefixed to every components rendered CSS this would result in a lot of code duplication in the end.
Probably not a big issue with gzip but still unnecessary.
Maybe we should rename it (maybe
custom-properties.scss?) and move it out of this folder (the namevariablesstood for 'SCSS variables') so that we don't mix contexts, since all the other files inside are imported by this_index.scss.#756 (comment)
I just realise that this might fix also the issue you had with
!important.Yes, there is definitely a lot of duplication and yes I'm sure that's why I had to use !important so much (it seems to be used quite a lot in scss, not just by me!).
Wasn't clear to me if that was introduced by my changes or pre-existing. I have to say I feel like I'd need a couple of hours and to draw a flow chart to figure out the flow of all these imports :) That said it looks like there was already some cleanup recently.
Added an issue so I remember to try this.
I've just removed all the '!important' declarations in colors.scss I needed before with no ill effect.
@ -19,0 +1,4 @@:root {/* Vikunja colors as CSS custom properties */--grey-50: hsl(210, 20%, 98%);Any specific reason why for the switch to hsl?
Well:
That makes sense =)
LCH is awesome! I always have to force myself to read it corectly because it looks like the German 'me' which is the word 'ich' (at the beginning of a sentence 'Ich' which looks really similar).
@ -19,0 +7,4 @@--grey-300: hsl(216, 12.2%, 83.9%);--grey-400: hsl(217.9, 10.6%, 64.9%);--grey-500-hsl: 220, 8.9%, 46.1%;--grey-500: hsla(var(--grey-500-hsl), 1);Is hsla needed here? With an opacity of
1it seems unnecessary2a65126a80@ -19,0 +33,4 @@--white-l: 100%;--white-a: 1;--white: hsla(var(--white-h), var(--white-s), var(--white-l), var(--white-a));--white-translucent: hsla(var(--white-h), var(--white-s), var(--white-l), 0.75);🔥
Not clear whether it's the use of variables (we're overriding the ones bulma-css-variables has set) or the fact that vscode seems to have formatted to tabs on paste :(
Fixed it in sublime text but that now formats to tabs on save too :(
I think we need a specific entry for scss files in editorconfig
Works for me...but do I include it in this PR?
Nahh all good! The 🔥 was in regard to the nice splitting mechanism of the individual color components / channels (not sure what it's called in hsl ?).
That's hot!
Regarding the formatting: we are looking into using prettier either way so I'm ignoring minor formatting stuff right now =)
Regarding
.editorconfig: maybe create a new separate issue =)Work work work ;)
@ -19,0 +42,4 @@--black: hsla(var(--black-h), var(--black-s), var(--black-l), var(--black-a));//$warning/$orange: #ff851b or hsl(27.9, 100%, 55.3%)--warning-h: 27.9deg !important;Can you add a comment why the
!importantis needed?"Because it wont't work without it" doesn't seem very informative :/
As per your earlier comment regarding @import location and order - I think it might be possible to resolve the need for those !important properties and I'd rather try that before commenting.
Yes I found out about that later =) The
calcin the comment below might even be possible without that!important@ -19,0 +67,4 @@--primary: hsla(var(--primary-h), var(--primary-s), var(--primary-l), var(--primary-a));// simulate sass lighten($primary, 30) by increasing lightness 30% to 73%--primary-light: hsla(var(--primary-h), var(--primary-s), 73%, var(--primary-a));Is it possible to use
calc()here?Ahh I saw the
!importantnowI did initially but really I think it just obfuscates the actual value, though I don't much like the this either.
So:
Works to give a 30% increase?
--primary-light is only used in src/views/tasks/TaskDetailView.vue and I've no idea what end result it has. Can't see what different values do. Any idea?
If we stick with hard coding a value I wonder if using the pattern bulma-css-variables uses would be clearer:
Thoughts? EDIT: have actually just changed it to the latter.
@ -19,0 +71,4 @@--primary-translucent: hsla(var(--primary-h), var(--primary-s), var(--primary-l), 0.5);}@media (prefers-color-scheme: dark) {Nest the
@mediaso we don't need to repeat:root.Good catch. Done
a258b938fa@ -6,0 +7,4 @@0 3px 6px hsla(var(--grey-500-hsl), .08);--shadow-lg: 0 15px 25px hsla(var(--grey-500-hsl), .12),0 5px 10px hsla(var(--grey-500-hsl), .05);// unused --shadow-xl: 0 20px 40px hsla(var(--grey-500-hsl), .2);good catch!
I think we should either remove it or uncomment it. Like this doesn't make sense (?).
The comment was to spark exactly this conversation :)
Should I remove it?
It's not used. It's not overriding something declared by bulma.
I vote for remove...
@konrad: do you remember the reason for shadow?
No real reason. When I added the shadows, I added a few variants to have a bit more flexibility. If it's not used anywhere I think it is fine to remove.
Removed shadow-xl
92b2010cdc@ -6,0 +11,4 @@}@media (prefers-color-scheme: dark) {:root {Nest the
@mediaso we don't need to repeat:root. (2)Done
a258b938fa@ -7,3 +1,2 @@$primary: $blue;$info-invert: $white;$info-invert: #fff;Might make sense to add a comment here why this is still defined as hex
Doesn't appear to be used in vikunja code and is a duplicate of the var declared by bulma-css-variables so I removed it instead
aff7a1f2ea@ -637,3 +637,3 @@.bucket {background-color: $bucket-background;background-color: var(--bucket-background);This seems like a mistake:
$bucket-backgroundis still defined as SASS variable.I think it makes sense to keep it that way: Compiled it will resolve in
var(--grey-100)which seems fineYep, reverted
0191f12db7Though declaring it to be the same color as what's behind it seems pointless - hangover from previous kanban design?
@konrad: any idea?
It kind of comes from the kanban redesign, but is still required: When the list has a background, the buckets need to have a colored background.
I think we could remove the variable and swap its uses to use the same variable that's used for the overall site background.
You mean the variable that was set for
$bucket-backgroundakavar(--grey-100)?Yes.
Removed $bucket-background
54f1271ab9@ -686,3 +686,3 @@.button {background: $bucket-background;background: var(--bucket-background);Same here
Reverted.
@ -7,3 +7,3 @@:focus {box-shadow: 0 0 0 2px rgba($primary, 0.5);box-shadow: 0 0 0 2px var(--primary-translucent);Doesn't
rgba(var(--primary), 0.5)work?No, since the var already includes
hsla()so resolved by the browser it would be double wrapped:See: https://kolaente.dev/vikunja/frontend/pulls/954/files#issuecomment-18553
rgba()also just accepts r, g, b, a values. SASS would render this as as the css rgba() and not the SCSSrgba()helper since it includes a custom-property which can not be resolved by SASS at build time.Given that --primary-translucent is only used twice (both in theme.scss, lines 8 & 18), it might make more sense to use the existing primary-* vars to build those 2 instances.
or do what I've done with some of the greys and define a 'naked' --primary-hsl that we can drop in:
I think the latter is most readable, and doesn't make the alpha value hard to see.
Thoughts @konrad @dpschen ?
I think the second one makes most sense.
I just realize that we have to define that
--primary-hsljust for that usecase. Then we could also just keep with the--primary-translucent.Because of that I would prefer the first solution. 🤷♂️
I created --primary-hsl as described above
6c59567ebf@ -19,0 +6,4 @@--grey-200: hsl(220, 13%, 91%);--grey-300: hsl(216, 12.2%, 83.9%);--grey-400: hsl(217.9, 10.6%, 64.9%);--grey-500-hsl: 220, 8.9%, 46.1%;Why is there
grey-500andgrey-500-hsl? If they are the same, why are there two of them?As far as I understand
grey-500is directly usable inside a css color property.E.g:
While
grey-500-hslare just the pure hsl values with the intend to use them in different contexts. E.g.:Makes sense. Then maybe there should be variables like these for all color variants? To make it consistent?
Yes, that's true. Especially since bulma-scss-variables does the same for all predefined ones.
Yes, @dpschen is right, I picked up that approach from bulma-css-variables and it's for adding the alpha where needed.
@konrad the reason I only added them for a couple of greys is because those are the only greys where an alpha variant is actually used. I get the desire for uniformity and consistency, but is it worth declaring unused variables?
(Technically I should remove the dark mode --grey-300-hsl because that's not currently used in the PR either, was the result of some experimenting.)
It probably is not. Let's say as long as there's only a few variants where a
-hslvariant makes sense there's no need to add one for each color. As long as maybe more than half of the variants are used as-is (without a-hslvariant) I'm fine with adding them on a case-by-case basis when they are needed.@ -19,0 +13,4 @@--grey-800: hsl(215, 27.9%, 16.9%);--grey-900: hsl(220.9, 39.3%, 11%);--light-background: var(--grey-100);We should call this just
--background-coloror--site-background-color.I've renamed the --light-background variable to --site-background in
a45c6cdf6b@ -19,0 +33,4 @@--white-l: 100%;--white-a: 1;--white: hsla(var(--white-h), var(--white-s), var(--white-l), var(--white-a));--white-translucent: hsla(var(--white-h), var(--white-s), var(--white-l), 0.75);--white-translucenthas a different opacity than--primary-translucent. I think it should either be the same kind of translucent or there should be variants of it like--white-opacity-75or similar. On the other hand, since we already defined the hsl parts for it we could probably just use those in combination with a different opacity value directly where it's required.Then the other thing is that a variable name like
whiteimplies that it contains some kind of white. In a dark color scheme that would probably contain something else entirely sowhitewouldn't actually contain white. Maybe it's a better idea to use a variable name that reflects what this is used for?The same applies to the greys but I don't really have a good idea for those. I noticed that you did this already for red/green/orange, I think that's the way to go.
That makes most sense to me. There are very few instances of translucent colours at present.
Unfortunely we inherit those from bulma/bulma-css-variables, so I think we're stuck with them for now. I know it seems odd, but you get used to it.
I think you already have the solution just think of --grey-100 as 10% dark in light mode and 10% light in dark mode.
An easy change because bulma/bulma-css-variables already defines those danger/success/warning variables. Bulma Colors docs.
@ -19,0 +71,4 @@--primary-light: hsla(var(--primary-h), var(--primary-s), var(--primary-light-l), var(--primary-a));--primary-translucent: hsla(var(--primary-h), var(--primary-s), var(--primary-l), 0.5);@media (prefers-color-scheme: dark) {Maybe it would make sense to group all dark scheme adjustments (and the base light versio) in separate stylesheets instead of seperating by color or shadows?
I think this is fine. Because we might need adjustments deep in components for dark mode either way.
I think it would be wrong to separate those styles from the components.
That's something I didn't think about. Makes sense.
I think this already looks quite good! Nice job :)
Here's a few things I noticed about the dark mode:
I think none of these are blocking for this PR, just to note them. I'd be fine to merge this without these things and follow-up with another PR to fix them.
@konrad: Some of these issues might be solved by:
adrinux/frontend#3
@konrad This is exactly the sort of extra eyes feedback I was hoping for here :)
With one exception:
I mean't to add a point in the initial PR description above about the editor but forgot.
I specifically avoided tinkering with the editor because it's a major component in itself:
It would be nice to tackle the editor toolbar, but I'd rather do that seperately in a later PR.
Sure, sounds like a good idea 🙂
Hi adrinux!
Thank you for creating a PR!
I've deployed the changes of this PR on a preview environment under this URL: https://954-bulma-css-variables--vikunja-frontend-preview.netlify.app
You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.
Have a nice day!
Current status of these:
Done.
Done.
Mutual agreement to postpone.
Looked into this - Chrome (the beta at least) is overriding the button colors with it's own internal stylesheet, so even the current light mode button color only applies to firefox. Possibly something to do with :autofill css pseudo class, haven't got to the bottom of this yet.
Quite a lot to fix in this pop-up.
Still Todo.
The specific 'Delete' button in the image should probably be fixed by adding class .is-danger?
Still Todo.
Done. A much more subtle border now.
If you look carefully they're actually slightly heavier looking in light mode too - so something else likely needs addressed rather than a dark mode override.
Still Todo. Need to look deeper at this.
I think we need the logo text to be the right color and that depends on #971 at the moment.
And the remaining big issue is adding UI to switch dark mode on or off permanently - that will also require removing the prefers-dark media query currently in this PR and replacing it with a .dark-mode class.
I created another branch that adds the color scheme settings:
https://kolaente.dev/dpschen/frontend/src/branch/feature/add-color-scheme-setting (branch)
817845a70b(commit)Wow, you really are on 🔥 Dominik!
Works really well!
Have merged @dpschen color scheme switch into my dark mode branch.
Does it work if we'd define the text color with the
:placeholderpseudo class globally for everything?Yup.
@ -2,3 +2,3 @@<div class="no-auth-wrapper"><div class="noauth-container"><Logo width="400" height="117" /><Logo class="logo" width="400" height="117" />Maybe setting the logo text color should be done in the
Logocomponent directly?I assumed so and added scoped scss in there but it had no effect - possibly my lack of vue experience - is it neccessary to tell vue it has to render scss in logo.vue?
That said this is only on the log on page. Once logged in the logo class is on the link wrapping the logo and svg inherits color set on that...
Did you tell it the content in the style tag was in scss with the
lang="scss"attribute?This should definately happen inside the Logo component, else we'll have to repeat the styles all over.
Yes I added lang="scss". It was late on a Saturday night so maybe I missed something but I literally copied the entire style tag from another vue file, and added the logo class to the template section.
I even killed and restarted yarn serve. So why didn't it work? Traditional css would say I need a more specific selector, but this is scoped so 🤷
Yes it should happen inside the logo component. And there are a pile of pre-existing logo styles that need moved there that are currently in other files.
When you check the dom: Does with that style the class
logoapply to the SVG?Because if there is a class that defines the
--logo-text-colorI don't see a reason why scoping should change anything (since currentColor inherits)?Other then that: there is also the
:deep()selector that helps styling deep children (basically stuff that is inside child components). But I don't think it should be necessary.Solved in
4133363fe1WIP: Migrate to bulma-css-variables and introduce dark modeto Migrate to bulma-css-variables and introduce dark modeRemoved WIP after fixing the logo text colour.
@konrad Points 4,5 & 7 in #954 (comment) still need work but can get their own issues as suggested.
Scratch that, it seems no specific color was set for the results button text, presumably that's why Chrome was falling back to internal styles. Fixed now.
Yeah...I haven't figured out how yet =D
Feel like I'm missing some broswer tools for vue that would tell me which template needs edited.
Down to 5 & 7 now.Down to point 5.
The button-related stuff should be fixed in the button component for the "tertary" style, that's located at
src/components/input/button.vue.The vue devtools extension can tell you what components are used where.
So that's all that's missing?
Turns out I had the non-beta version installed but it wasn't working :)
Installed the beta version.
There may yet be other broken parts tucked away in there somewhere. That's the last that I'm aware of.
I think there other non-broken but not perfect bits that need some action, but those veer into taste and design decisions. Like the too dark (to my eyes) hover state on buttons.
Then there pre-existing things like the active state of input border colour being --link instead of --primary, something I'd never noticed as a user but is way more obvious in dark mode.
Should those design changes belong in this PR?
Yeah the stable version has issues with Vue 3.
Maybe we should just set
--linkto--primaryonce and for all?I'd say if those are things that are required to fix for dark mode to work, then yes. I think this PR is quite big already, might as well do follow-up PRs for everything not related to the switch to bulma-css-vars and adding a basic dark color scheme that "looks fine".
I'm not so happy with the component specific var definitions in
colors.scsse.g.--card-border-color. But I also don't have a good solution for these right now.The main issue that I have is that the scoped styles prevent us from accessing the global
.darkclass that is active on thehtmlelement in dark mode (Because.darkeither points to a scoped.darkclass or the nested classes will not be scoped when using:globa()).I have an idea, but that needs a change in vues sfc-compiler. I'm currently preparing an issue for this.
For now let's keep the styles as they are, just wanted to have that mentioned.
Still don't understand what's required with Vue to mark that delete button 'is-danger'.
But I have just pushed
359a6b17d9which at least prevents the text from becoming invisible on hover.I think that's the last visibly broken part made useable.
RE:
I wrote something: https://github.com/vuejs/rfcs/discussions/414
Maybe you can give an upvote :D
@ -144,6 +144,12 @@ $vikunja-nav-logo-full-width: 164px;.logo {display: none;color: var(--logo-text-color);Put this in logo.vue
Done!
@ -146,1 +146,4 @@display: none;color: var(--logo-text-color);&:hover, &:focus {This also in logo.vue
Done!
@ -38,1 +38,4 @@}.logo {color: var(--logo-text-color);Move to logo.vue
Done!
@ -0,0 +17,4 @@--site-background: var(--grey-100);--scheme-main: var(--white);// Overrides of Bulma defaultsThis comment is a bit unclear: where do the overwrites of bulmas defaults end?
Tightened up the whitespace so all the overrides are in one block. That clearer?
Tbh I liked that they were devided by an empty line. How about an end comment, something like:
@dpschen You can have end comment if you prefer :) And empty lines back?
While we're talking about this block, how would feel about removing the comment lines with the old color name/hex etc?
Mostly in to show what I'd done but they seem superfluous now.
Added end comment, added whitespace back, removed extra hsl in comment
@ -0,0 +26,4 @@--grey-light: var(--grey-300);--grey-lighter: var(--grey-200);--grey-lightest: var(--grey-100);// --white-ter: whitesmoke;I didn't get what these uncommented values are for?
Thought I might need to override them when I started, removed them now.
@ -0,0 +71,4 @@// simulate sass lighten($primary, 30) by increasing lightness 30% to 73%--primary-light-l: 73%;--primary-a: 1;--primary-hsl: 216.5deg, 100%, 54.9%;Reuse
--primary-h,--primary-sand--primary-ls valuesDone
@ -9,3 +9,3 @@// FIXME: move to pagination component.pagination-link:not(.is-current) {background: $light-background;background: var(--site-background);It seems wrong to use here the
site-backgroundsince this is a link – better justvar(--grey-100)?That seems perfectly sensible. Done.
@ -843,3 +843,4 @@}&:not(:disabled) {// HACK: these styles are repeated from bulma-css-variables/sass/form/shared.sassRemove hack and replace colors with vars directly
Done!
@ -61,0 +62,4 @@}/* Close icon SVG uses currentColor, change the color to keep it visible */@media (prefers-color-scheme: dark) {Respect
.darkclass.Done!
This one is a little more complicated. The tertary action is defined in the
<create-edit/>component here and could be anything, not just delete. Because it only allows passing in props with a button action and text we can't really style it. In order to re-style that class we would need to change that component to either allow adding arbitrary classes to its buttons or change it to use slots instead. Since that's not something strictly related to themeing I think we could leave it as-is for now, as long as the original issue with the tertary action class not being visible at all is solved.Leave it as is for now.
I wanted to tackle a bit how this works either way =)
Looks good to me 💪
@konrad: what do you think?
I've noticed three more things (not a blocker for merging this though):
Looks good to me! Thanks a lot for your work.
Moved my notes from the comment above to #1063, will merge this once CI passes.
🎉
Yay!! =)