Date picker widget unreadable in modal in light mode #1074

Open
opened 1 week ago by adrinux · 12 comments
adrinux commented 1 week ago

I think #1070 merge has caused issues in light mode.

Styles changed in #1070 need moved into a nested &.dark {...} selector and the original light mode styles returned?

(Not every change in the dark mode is a simple switch, text in particular.)

EDIT: this is only happening when using the date widget in the modal view!

I think #1070 merge has caused issues in light mode. Styles changed in #1070 need moved into a nested ```&.dark {...}``` selector and the original light mode styles returned? (Not every change in the dark mode is a simple switch, text in particular.) EDIT: this is only happening when using the date widget in the modal view!
Collaborator

You are right. Was not very well tested by me.

You are right. Was not very well tested by me.
adrinux changed title from Date picker widget unreadable in light mode to Date picker widget unreadable in modal in light mode 1 week ago
konrad added the
kind/bug
label 1 week ago
Owner

This seems to be related to --text not being defined. I think this is a variable that should be defined from bulma-css-vars?

This seems to be related to `--text` not being defined. I think this is a variable that should be defined from `bulma-css-vars`?
Owner

Put up a PR with a fix: #1079

Put up a PR with a fix: https://kolaente.dev/vikunja/frontend/pulls/1079
Poster

This seems to be related to --text not being defined. I think this is a variable that should be defined from bulma-css-vars?

So that's --text and --link that were missing...I can't help but think something deeper is wrong.

I swear I would have noticed before now if --text was missing. I'm wondering if I clobbered some of the sass imports @dpschen provided in PRs to my branch during an upstream merge.

> This seems to be related to `--text` not being defined. I think this is a variable that should be defined from `bulma-css-vars`? So that's --text and --link that were missing...I can't help but think something deeper is wrong. I swear I would have noticed before now if --text was missing. I'm wondering if I clobbered some of the sass imports @dpschen provided in PRs to my branch during an upstream merge.
Poster

There are quite a lot of undefined custom properties. Not just those two.

Both --text and --link are set in bulma-css-variables/sass/utilities/derived-variables.sass and those derived variables are derived from inital-variables.sass.

So they exist, but are they imported?

src/styles/common-imports.scss brings in bulma-css-variables/sass/utilities/_all

But I can't see where common-imports.scss is imported or processed with sass, maybe @dpschen can shed some light on it, but that seems to be where all our missing variables are.

I would suggest not merging sticking plaster PR's like #1079 till this is resolved.

There are quite a lot of undefined custom properties. Not just those two. Both --text and --link are set in ```bulma-css-variables/sass/utilities/derived-variables.sass``` and those derived variables are derived from inital-variables.sass. So they exist, but are they imported? ```src/styles/common-imports.scss``` brings in ```bulma-css-variables/sass/utilities/_all``` But I can't see where common-imports.scss is imported or processed with sass, maybe @dpschen can shed some light on it, but that seems to be where all our missing variables are. I would suggest not merging sticking plaster PR's like #1079 till this is resolved.
Owner

I would suggest not merging sticking plaster PR's like #1079 till this is resolved.

Yeah that sounds like a good idea. I think I've noticed other variables being undefined too, really looks like something is wrong here.

> I would suggest not merging sticking plaster PR's like #1079 till this is resolved. Yeah that sounds like a good idea. I think I've noticed other variables being undefined too, really looks like something is wrong here.
Collaborator

I have an idea what this issue is about.

But I can't see where common-imports.scss is imported or processed with sass, maybe @dpschen can shed some light on it, but that seems to be where all our missing variables are.

It's imported via the sass additionalData option in the vite.config.js

There are quite a lot of undefined custom properties.

@adrinux Which other variables then the mentioned are missing? Or more insterestingly: in which files are the missing variables defined?

I have an idea what this issue is about. > But I can't see where common-imports.scss is imported or processed with sass, maybe @dpschen can shed some light on it, but that seems to be where all our missing variables are. It's imported via the sass `additionalData` option [in the vite.config.js](https://kolaente.dev/vikunja/frontend/src/branch/main/vite.config.js#L13) > There are quite a lot of undefined custom properties. @adrinux Which other variables then the mentioned are missing? Or more insterestingly: in which files are the missing variables defined?
Poster

I have an idea what this issue is about.

If I've understood it correctly it's to do with avoiding duplicating the css for every vue component?

It seems that the sass variables are correctly getting filled in as css variables during preprocessing but the bulma default values for those css variables don't get passed though?

I believe the chain is:

vite.config.js < src/styles/common-imports.scss < bulma-css-variables/sass/utilities/_all < bulma-css-variables/sass/utilities/derived-variables.sass < inital-variables.sass

@adrinux Which other variables then the mentioned are missing? Or more insterestingly: in which files are the missing variables defined?

--link, --text and other variable values are set in derived-variables.sass,
with the colour (and other variable) values they use defined in initial-variables.sass

So do the derived variable's values need to be imported into and set in the global.scss scope too?

Or if we're overriding half these colour variables to vikunja specific values anyway is it better to just put them all in custom-properties/colors.scss and be done?

> I have an idea what this issue is about. If I've understood it correctly it's to do with avoiding duplicating the css for every vue component? It seems that the sass variables are correctly getting filled in as css variables during preprocessing but the bulma default values for those css variables don't get passed though? I believe the chain is: ``` vite.config.js < src/styles/common-imports.scss < bulma-css-variables/sass/utilities/_all < bulma-css-variables/sass/utilities/derived-variables.sass < inital-variables.sass ``` > @adrinux Which other variables then the mentioned are missing? Or more insterestingly: in which files are the missing variables defined? --link, --text and other variable values are set in derived-variables.sass, with the colour (and other variable) values they use defined in initial-variables.sass So do the derived variable's values need to be imported into and set in the global.scss scope too? Or if we're overriding half these colour variables to vikunja specific values anyway is it better to just put them all in custom-properties/colors.scss and be done?
Collaborator

If I've understood it correctly it's to do with avoiding duplicating the css for every vue component?

Yes

... but the bulma default values for those css variables don't get passed though?

I think they are. But only for the global.scss. Since the component styles are isolated they only have the SCSS variables available that get defined in common-imports.scss. CSS Custom properties should be available globally – if this is not the case maybe I miss something?

@adrinux Maybe you can try to fix it by increasing the specifiy of the custom properties so they always overwrite the ones defined by bulma-css-variables:

// in `custom-properties.scss`
html:root {} // Selector Specificity: (0, 1, 1)

// instead of

:root {} // Selector Specificity: (0, 1, 0)

So do the derived variable's values need to be imported into and set in the global.scss scope too?

Yes

Or if we're overriding half these colour variables to vikunja specific values anyway is it better to just put them all in custom-properties/colors.scss and be done?

Might also work.


Another way that might help fix this could be if we use bulvar and bulvar starts using the @use sass module syntax. I suggested this already.

> If I've understood it correctly it's to do with avoiding duplicating the css for every vue component? Yes > ... but the bulma default values for those css variables don't get passed though? I think they are. But only for the `global.scss`. Since the component styles are isolated they only have the SCSS variables available that get defined in `common-imports.scss`. CSS Custom properties should be available globally – if this is not the case maybe I miss something? @adrinux Maybe you can try to fix it by increasing the specifiy of the custom properties so they always overwrite the ones defined by bulma-css-variables: ```scss // in `custom-properties.scss` html:root {} // Selector Specificity: (0, 1, 1) // instead of :root {} // Selector Specificity: (0, 1, 0) ``` > So do the derived variable's values need to be imported into and set in the global.scss scope too? Yes > Or if we're overriding half these colour variables to vikunja specific values anyway is it better to just put them all in custom-properties/colors.scss and be done? Might also work. --- Another way that might help fix this could be if we use bulvar _and_ bulvar starts using the `@use` sass module syntax. [I suggested this already](https://github.com/daniil4udo/bulvar/issues/38#issuecomment-980437332).
Collaborator

I just realized that

html:root {}

doesn't select SVG elements. So custom properties would not be available in there.
I guess a better way to increase specifiy would be to repeat the selector:

:root:root {} // Selector Specificity: (0, 2, 0)

By the way this is also why it's usually recommended to use :root instead of html for custom properties that should be globally available. Wasn't aware of that =)

I just realized that ```scss html:root {} ``` doesn't select SVG elements. So custom properties would not be available in there. I guess a better way to increase specifiy would be to repeat the selector: ```scss :root:root {} // Selector Specificity: (0, 2, 0) ``` By the way this is also why it's usually recommended to use `:root` instead of `html` for custom properties that should be globally available. Wasn't aware of that =)
Poster

@adrinux Maybe you can try to fix it by increasing the specifiy of the custom properties so they always overwrite the ones defined by bulma-css-variables:

@dpschen This makes no sense to me. The error we see in browser dev tools is that '--link is undefined' and so on for other bulma variables too.

It's not a question of needing to override them - no value has been set for them!

And the reason we're seeing these problems such as with the date picker is that those elements were relying on bulma variables rather than anything ever being set for them in vikunja.

> @adrinux Maybe you can try to fix it by increasing the specifiy of the custom properties so they always overwrite the ones defined by bulma-css-variables: @dpschen This makes no sense to me. The error we see in browser dev tools is that '--link is undefined' and so on for other bulma variables too. It's not a question of needing to override them - no value has been set for them! And the reason we're seeing these problems such as with the date picker is that those elements were relying on bulma variables rather than anything ever being set for them in vikunja.
Collaborator

Yeah maybe I had a different imagination of that problem.

Nevertheless I still think that the @use "way" might solve these kind of problems.
And the bulvar lib now seems to have some progress in that regard: https://github.com/daniil4udo/bulvar/issues/38#issuecomment-981061391

Yeah maybe I had a different imagination of that problem. Nevertheless I still think that the `@use` "way" might solve these kind of problems. And the bulvar lib now seems to have some progress in that regard: https://github.com/daniil4udo/bulvar/issues/38#issuecomment-981061391
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.