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

Add colorized bracket highlighting colours #132494

Conversation

TabithaLarkin
Copy link
Contributor

@TabithaLarkin TabithaLarkin commented Sep 6, 2021

This PR fixes #132493

As suggested in this comment by @hediet.

This shows an preview of what these colours would look like (generated from adding these settings to my local settings.json file.

image

edit by @hediet:

With default colors:

Without bracket pair colorization:

@hediet
Copy link
Member

hediet commented Sep 6, 2021

Thanks! Can you post a before/after screenshot of the same code region, so that it becomes very clear what the difference is?

@hediet hediet self-assigned this Sep 6, 2021
@hediet hediet added this to the September 2021 milestone Sep 6, 2021
@TabithaLarkin
Copy link
Contributor Author

Ah, I've already added those screenshots to the issue that I raised, with the bracket colorization off and on too.

@hediet
Copy link
Member

hediet commented Sep 6, 2021

@roblourens what do you think (I think it is your theme)?

@roblourens
Copy link
Member

Not me :)

@roblourens
Copy link
Member

roblourens commented Sep 9, 2021

I don't think that individual themes have owners. You could run it by @misolori if you want another opinion. Looks fine to me though.

@hediet
Copy link
Member

hediet commented Sep 10, 2021

@misolori What do you think?

@miguelsolorio
Copy link
Contributor

miguelsolorio commented Sep 10, 2021

Here's a snapshot of the theme colors + bracket colors with the original vs the proposed colors:

CleanShot 2021-09-10 at 09 10 02@2x

I'd probably keep the bracket colors closer to the original so this is what I would suggest:

		"editorBracketHighlight.foreground1": "#b58800",
		"editorBracketHighlight.foreground2": "#d33682",
		"editorBracketHighlight.foreground3": "#0B75BF",

CleanShot 2021-09-10 at 09 12 54@2x

Before

CleanShot 2021-09-10 at 09 13 50@2x

After

CleanShot 2021-09-10 at 09 16 20@2x

@TabithaLarkin
Copy link
Contributor Author

The reason I selected the white colour was because that was what the standard colour of the brackets without any bracket pair colorization. I would also caution against using the blue colour as it's the same colour as the text, which somewhat removes the benefit of colour differences in IDEs.

image

@miguelsolorio
Copy link
Contributor

The reason I selected the white colour was because that was what the standard colour of the brackets without any bracket pair colorization

Doesn't this conflict with the feature? I think we'd want to mirror the original as close as possible while still matching the theme

I would also caution against using the blue colour as it's the same colour as the text, which somewhat removes the benefit of colour differences in IDEs

Fair point, I think an alternate color can work here as well

@hediet
Copy link
Member

hediet commented Sep 13, 2021

Doesn't this conflict with the feature? I think we'd want to mirror the original as close as possible while still matching the theme

I think it is actually not a bad idea to use white as one of three (or more) colors for specific themes: This feature is all about being able to see matching bracket pairs, not neccessarily about highlighting brackets from text.
The original extension does not support themeable colors anyway.

@hediet hediet modified the milestones: September 2021, Backlog Sep 14, 2021
@hediet hediet removed their assignment Oct 11, 2021
@TabithaLarkin
Copy link
Contributor Author

@misolori Is there anything I can do to get this over the line? What's the blocker here?

Copy link
Contributor

@miguelsolorio miguelsolorio left a comment

Choose a reason for hiding this comment

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

Thanks for the ping, LGTM let's get this merged and see how it looks like on Insiders. Thank you!

@miguelsolorio miguelsolorio merged commit fba6ce5 into microsoft:main Oct 19, 2021
@miguelsolorio miguelsolorio modified the milestones: Backlog, October 2021 Oct 19, 2021
@TabithaLarkin TabithaLarkin deleted the TabithaLarkin/DarkSolarizedColorizedBrackets branch October 20, 2021 01:45
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add bracket pair colorization settings for Solarized Dark theme
4 participants