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

[theme] allow variables in the color theme definition file #105247

Open
rotem-bar opened this issue Aug 23, 2020 · 10 comments
Open

[theme] allow variables in the color theme definition file #105247

rotem-bar opened this issue Aug 23, 2020 · 10 comments
Assignees
Labels
feature-request Request for new features or functionality themes Color theme issues
Milestone

Comments

@rotem-bar
Copy link
Contributor

Support of using already defined colors in a theme's theming rule will make themes a much more pleasant and easier experience in my opinion, instead of having a bunch of hexes all over the place or having to change all the occurrences when changing a widely used color.

Example:

{
	"$schema": "vscode://schemas/color-theme",
	"colors": {
		"editor.foreground": "#FFFFFF"
	},
	"tokenColors": [	
		{
			"name": "Variable",
			"scope": [
				"variable",
				"meta.definition.variable.name",
				"support.variable"
			],
			"settings": {
				"foreground": "#FFFFFF"
			}
		}
	]
}

Can be achieved using:

{
	"$schema": "vscode://schemas/color-theme",
	"colors": {
		"editor.foreground": "#FFFFFF"
	},
	"tokenColors": [	
		{
			"name": "Variable",
			"scope": [
				"variable",
				"meta.definition.variable.name",
				"support.variable"
			],
			"settings": {
				"foreground": {
					"$color": "editor.foreground"
				}
			}
		}
	]
}

Syntax for this feature is up for discussion,
Can work on a PR for this if its relevant.

@aeschli aeschli changed the title Using already defined colors in a theme's theming rules for token colors settings [theme] allow variables in the color theme definition file Aug 24, 2020
@aeschli
Copy link
Contributor

aeschli commented Aug 24, 2020

Your request is kind of similar to #56855. It asks for color variables but for the color configurations in the user settings. (workbench.colorCustomizations, editor.colorCustomizations and editor.semanticTokenColorCustomizations),

One can think of a solution that works for themes and settings, e.g. like in the snippet below, I'm a bit afraid that things get quite complex. So I wouldn't mind looking at a solution just for the color theme file, as you propose. If you have interest to look at it, I'd be open for a PR. But we first would need to agree on the syntax.

Here's a attempt based on the proposals from #56855:

	"colorPalette": {
		"color1": "#fa0000"
	}
	"colors": {
		"activityBar.foreground": "$color1"
	},
	"tokenColors": [	
		{
			"name": "Variable",
			"scope": [
				"variable",
				"meta.definition.variable.name",
				"support.variable"
			],
			"settings": {
				"foreground": "$color1"
			}
		}
	]

@aeschli aeschli added themes Color theme issues feature-request Request for new features or functionality labels Aug 24, 2020
@aeschli aeschli added this to the Backlog milestone Aug 24, 2020
@rotem-bar
Copy link
Contributor Author

I have to agree with your syntax, separate object for variables does seem like a better approach, and having it as a string pattern for variables makes it much easier implementation-wise instead of using an object.

@iDad5
Copy link

iDad5 commented Sep 11, 2022

Can I ask what the status on this is?

Also I would humbly suggest to think about a syntax that is more like CSS custom properties, as I guess that most theme developers have a background in that:

"colorPalette": {
	"--color1": "#fa0000",
            "--color2": "#fafa0666"
}
"colors": {
	"activityBar.foreground": "var(--color1)"
},
"tokenColors": [	
	{
		"name": "Variable",
		"scope": [
			"variable",
			"meta.definition.variable.name",
			"support.variable"
		],
		"settings": {
			"foreground": "var(--color2)"
		}
	}
]

It would maybe at some point in the future allow for more complex but very useful addition like hsl colors and separated lightness values or even transparency like CSS color palettes can do.

But that's just an idea I would love to see the support for color palettes very much regardless of the syntax.

@aeschli
Copy link
Contributor

aeschli commented Sep 12, 2022

@iDad5 Sorry, we currently have no plans to implement such a feature. But we're open for a PR.

@iDad5
Copy link

iDad5 commented Sep 12, 2022

@iDad5 Sorry, we currently have no plans to implement such a feature. But we're open for a PR.

Ok, as I have admitted earlier, I do not have real experience in contribution to such a large project in that way, but I do consider trying it, as the task itself - at least from the outside seems doable and I like a challenge. How would we go about ist?
Could you offer some guidance on where to best start, what better not to touch etc. or would I just have to try on my own and at the end you would judge if it's acceptable?

@aeschli
Copy link
Contributor

aeschli commented Sep 13, 2022

if (resources.extname(themeLocation) === '.json') {
is where the theme file is read.

Check out https://github.com/microsoft/vscode/wiki/How-to-Contribute on how to set up the repo

@iDad5
Copy link

iDad5 commented Sep 13, 2022

@aeschli Thank you for your reply!

Looks like a good starting point, I've already had a glance at How-to-Conrtibute and even though I'll have a learning curve, it seems doable.

the colorThemeData.ts and the entry point you provided is very helpful. Thanks.

I know that you are very busy and I hate to trouble you with questions, so if anyone else can help me out I would be grateful too:

Before I would start giving it a try my questions would be:

  • is the syntax I suggested an acceptable one, or would you prefer an other one (e.g. does "colorPalette" work?)
  • my proposition would be to filter/parse all the data once successfully received (around line 711 I'd guess) trough some colorPalett class, that returns the content with the color variable values replaced with the resulting HexColors. That would keep open the option to, at some point even use hsl etc. or more complex variable substitution without touching the rest of the application. Of course if no colorPalette is defined it would immediately return the original...

Is that an acceptable way to go on?

@aeschli
Copy link
Contributor

aeschli commented Sep 14, 2022

Thanks @iDad5, sounds good.

Maybe name it ColorParser. It would be constructed with the colorpalette and has a method 'parse` that knows about the syntax.

On the variable syntax, I'm more in favor of the dollar syntax. Either $variable, or ${variable}. We don't use the css variable syntax in our confguration files so far. That we use CSS under the hoods of VS Code is more like an implementation detail.

@iDad5
Copy link

iDad5 commented Sep 15, 2022

I would propose to call it ColorPaletteParser then, if that is ok, as I would like to see (at some point) the option to define colors not only in rgb-hash notation but also in hsl wich is much more intuitive for use in color-themes.

Provided that we are using json the $variable variant would be for me the way to go then.
"colorPalette" as the property key is ok?

(Currently I'm testing out which way I would like to set up my environment for that development, so it'll take a little while before I actually will be starting programming...)

@d-mahard
Copy link
Contributor

Hello
Is there any update on this one? I'd be happy to give this a try and submit a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality themes Color theme issues
Projects
None yet
Development

No branches or pull requests

4 participants