Redefine all Bulma variables to fix undefined variables #1165

Merged
konrad merged 12 commits from adrinux/frontend:set-all-variables into main 2022-03-27 19:20:38 +00:00
Contributor

As per discussion in #1074

This will hopefully eventually be fixed at the root - with missing scss variables properly passed though, possibly by switching to Bulvar.

But in the mean time I experimented with add ALL the bulma css color related variables into color.scss

This fixes the issue with the date picker in light mode. Also has me wondering if it will allow removal of some of other little overrides we made to fix niggling issues with dark mode, so marking this WIP.

Not suggesting this ever be merged, just exploring.

Resolves #1497
Resolves #1074

As per discussion in https://kolaente.dev/vikunja/frontend/issues/1074 This will hopefully eventually be fixed at the root - with missing scss variables properly passed though, possibly by switching to Bulvar. But in the mean time I experimented with add ALL the bulma css color related variables into color.scss This fixes the issue with the date picker in light mode. Also has me wondering if it will allow removal of some of other little overrides we made to fix niggling issues with dark mode, so marking this WIP. Not suggesting this ever be merged, just exploring. Resolves #1497 Resolves #1074
adrinux added 7 commits 2021-12-09 13:41:23 +00:00
Owner

If this is merged, I think it would superseed #1079?

If this is merged, I think it would superseed https://kolaente.dev/vikunja/frontend/pulls/1079?
Member

Hi adrinux!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1165-set-all-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!

Beep boop, I'm a bot.

Hi adrinux! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1165-set-all-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! > Beep boop, I'm a bot.
Author
Contributor

If this is merged, I think it would superseed #1079?

Well, yes and no.

This brings back the default Bulma values for --text etc which apply in light mode and are overriden in dark mode.

But #1079 uses vikunja specific shades of grey for --text etc in light mode.

Both fix #1074 but #1079 is a design choice too. Probably need some side by side screenshots to be clear what the difference is.

EDIT: I'm making screenshots

> If this is merged, I think it would superseed https://kolaente.dev/vikunja/frontend/pulls/1079? Well, yes and no. This brings back the default Bulma values for --text etc which apply in light mode and are overriden in dark mode. But #1079 uses vikunja specific shades of grey for --text etc in light mode. Both fix #1074 but #1079 is a design choice too. Probably need some side by side screenshots to be clear what the difference is. EDIT: I'm making screenshots
Author
Contributor

Here's combined screenshots of #1079 (left) and this PR #1165 (right) - if there is a difference in text colour it's imperceptible.

EDIT: On closer inspection some titles (Hi demo!, Last Viewed and Current Tasks for example) have much darker text in #1079 vs #1165 and in general text is darker in #1079.

Most obvious difference here is that the list card link text gains --link colour in this PR. In #1079 and I think main, it's overriden by a scoped --title colour. It is grey in the current release too because its span element.

So this approach may cause problems as well as fix them.

EDIT: Between the existing release and current head of the main branch the list card html changed from 'span' to 'a' - it should have link colour.

Here's combined screenshots of #1079 (left) and this PR #1165 (right) - if there is a difference in text colour ~~it's imperceptible~~. EDIT: On closer inspection some titles (Hi demo!, Last Viewed and Current Tasks for example) have much darker text in #1079 vs #1165 and in general text is darker in #1079. Most obvious difference here is that the list card link text gains --link colour in this PR. In #1079 and I think main, it's overriden by a scoped --title colour. It is grey in the current release too because its span element. ~~So this approach may cause problems as well as fix them.~~ EDIT: Between the existing release and current head of the main branch the list card html changed from 'span' to 'a' - it should have link colour.
Owner

But #1079 uses vikunja specific shades of grey for --text etc in light mode.

Both fix #1074 but #1079 is a design choice too. Probably need some side by side screenshots to be clear what the difference is.

I'm fine merging this and closing the other one. I made a design choice in the other PR yes but didn't put much though in it.

We should change the color of the list cards though, since that's also broken right now. Could be another PR.

> But #1079 uses vikunja specific shades of grey for --text etc in light mode. > > Both fix #1074 but #1079 is a design choice too. Probably need some side by side screenshots to be clear what the difference is. I'm fine merging this and closing the other one. I made a design choice in the other PR yes but didn't put much though in it. We should change the color of the list cards though, since that's also broken right now. Could be another PR.
Author
Contributor

I'm fine merging this and closing the other one. I made a design choice in the other PR yes but didn't put much though in it.

It would at least let things move forward. And they're easy enough to remove when the underlying problem gets sorted.

I'll tidy up some the comments a bit.

We should change the color of the list cards though, since that's also broken right now. Could be another PR.

Back to the normal text colour?

> I'm fine merging this and closing the other one. I made a design choice in the other PR yes but didn't put much though in it. It would at least let things move forward. And they're easy enough to remove when the underlying problem gets sorted. I'll tidy up some the comments a bit. > We should change the color of the list cards though, since that's also broken right now. Could be another PR. Back to the normal text colour?
Owner

It would at least let things move forward. And they're easy enough to remove when the underlying problem gets sorted.

Absolutely.

Back to the normal text colour?

Yeah exactly. The same way they look like in the current stable version.

> It would at least let things move forward. And they're easy enough to remove when the underlying problem gets sorted. Absolutely. > Back to the normal text colour? Yeah exactly. The same way they look like in the current stable version.
adrinux added 1 commit 2021-12-14 14:17:36 +00:00
adrinux added 1 commit 2021-12-14 14:32:00 +00:00
continuous-integration/drone/pr Build is passing Details
b83fe3163a
Tidy comments
adrinux added 1 commit 2021-12-14 23:07:10 +00:00
Author
Contributor

Back to the normal text colour?

Yeah exactly. The same way they look like in the current stable version.

Looks like that's already fixed, will remove the WIP prefix.

> > Back to the normal text colour? > > Yeah exactly. The same way they look like in the current stable version. Looks like that's already fixed, will remove the WIP prefix.
adrinux changed title from WIP: Redefine all Bulma variables to fix undefined variables to Redefine all Bulma variables to fix undefined variables 2021-12-15 12:23:18 +00:00
adrinux added 1 commit 2021-12-15 12:23:36 +00:00
continuous-integration/drone/pr Build is passing Details
6b6535ddd2
Merge branch 'main' into set-all-variables
konrad approved these changes 2021-12-15 13:16:19 +00:00
Owner

@dpschen I think we should merge this, anything to add?

@dpschen I think we should merge this, anything to add?
Member

Sry, lots to do! Trying to get to this as soon as possible!

Sry, lots to do! Trying to get to this as soon as possible!
konrad requested review from dpschen 2021-12-25 20:57:42 +00:00
dpschen was assigned by konrad 2021-12-25 20:57:49 +00:00
Owner

@dpschen Anything to add?

@dpschen Anything to add?
konrad added 1 commit 2022-03-27 16:08:07 +00:00
continuous-integration/drone/pr Build is passing Details
e91c24ab1a
Merge branch 'main' into set-all-variables
konrad merged commit a8fe2cdcbd into main 2022-03-27 19:20:38 +00:00
konrad deleted branch set-all-variables 2022-03-27 19:20:38 +00:00
Owner

Thanks! Sorry it took so long to merge.

Thanks! Sorry it took so long to merge.
This repo is archived. You cannot comment on pull requests.
No description provided.