Skip to content
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

Replacing global themed sass-vars to css-vars #4322

Merged
merged 5 commits into from
Nov 15, 2021
Merged

Replacing global themed sass-vars to css-vars #4322

merged 5 commits into from
Nov 15, 2021

Conversation

ixrock
Copy link
Contributor

@ixrock ixrock commented Nov 11, 2021

PR highlights:

  • Having default css-variables in app from generated default dark theme lens-dark.json
  • Better IDE/vscode css-variables support / auto-completion
  • More consistent styles overall
  • Possibility to get rid of sass in future at all (if postcss would be just enough)

Screenshot 2021-11-11 at 20 57 42

Recommended vscode extension for css-variables auto-complete:
https://marketplace.visualstudio.com/items?itemName=phoenisx.cssvar

And add this to settings.json for auto-scanning css-variables in the project from all possible files:

    "cssvar.files": [
        "./src/renderer/**/*.css",
        "./src/renderer/**/*.scss",
    ],

Signed-off-by: Roman <ixrock@gmail.com>
…lt theme colors

Signed-off-by: Roman <ixrock@gmail.com>
…lt theme colors

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock ixrock added enhancement New feature or request area/ui labels Nov 11, 2021
@ixrock ixrock requested review from jakolehm, Nokel81, aleksfront and a team November 11, 2021 19:14
@ixrock ixrock self-assigned this Nov 11, 2021
@@ -0,0 +1,136 @@

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this file should be committed, should it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should. It's used in app as default values for themed variables + used by editor's css-vars auto-completion.

P.S. Most vscode plugins for autocompleting css-variables support only defining exact file to read for variables, so could be useful too.


import fs from "fs-extra";
import path from "path";
import defaultBaseLensTheme from "../src/renderer/themes/lens-dark.json";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only dark?

Copy link
Contributor Author

@ixrock ixrock Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only? Because default could be just one as name says defaultBaseLensTheme (could be default white tought)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so this is only for the auto-complete

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be more colors in lens-light.json

Copy link
Contributor

@jim-docker jim-docker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing this:

Screen Shot 2021-11-12 at 12 54 24 PM

should be:

Screen Shot 2021-11-12 at 12 40 55 PM

I thought it was a problem in master too because after testing your branch I pulled master and ran make dev and got the same (bad) result. But when I ran make clean && make dev on master it was good. This build target should be part of make dev? Oh, it's probably just the transition between this PR and master, once it's merged there won't be this build issue moving forward. nvm


import fs from "fs-extra";
import path from "path";
import defaultBaseLensTheme from "../src/renderer/themes/lens-dark.json";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be more colors in lens-light.json

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock
Copy link
Contributor Author

ixrock commented Nov 15, 2021

@lensapp/lens-maintainers ready for re-review / PTALA

Added 2 missing theme colours to lens-dark.json: --layoutTabsLineColor, --canvasBackground .

@jim-docker light theme seems to be fixed for now:

image

Copy link
Contributor

@jim-docker jim-docker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -37,7 +37,7 @@
align-self: center;

.LineProgress {
color: $lensBlue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were lensBlue and lensMagenta defined somewhere?

Copy link
Contributor Author

@ixrock ixrock Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, they where defined in linking sass-vars to css-vars in deleted theme-vars.scss, e.g.

$lensBlue: var(--blue);
// ... other themed vars

@ixrock ixrock merged commit 4c83f5b into master Nov 15, 2021
@ixrock ixrock deleted the theme-css-vars branch November 15, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants