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

Support theming of active line number #42301

Merged
merged 3 commits into from
Feb 23, 2018
Merged

Support theming of active line number #42301

merged 3 commits into from
Feb 23, 2018

Conversation

usernamehw
Copy link
Contributor

Fixes #21825

@rebornix
Copy link
Member

@usernamehw I like the idea of using ~ to make it work, super neat! As we are already in endgame, I'll merge this PR next week and have it shipped in Feb. Thanks for your help!

@@ -19,6 +19,7 @@ export const editorCursorBackground = registerColor('editorCursor.background', n
export const editorWhitespaces = registerColor('editorWhitespace.foreground', { dark: '#e3e4e229', light: '#33333333', hc: '#e3e4e229' }, nls.localize('editorWhitespaces', 'Color of whitespace characters in the editor.'));
export const editorIndentGuides = registerColor('editorIndentGuide.background', { dark: editorWhitespaces, light: editorWhitespaces, hc: editorWhitespaces }, nls.localize('editorIndentGuides', 'Color of the editor indentation guides.'));
export const editorLineNumbers = registerColor('editorLineNumber.foreground', { dark: '#5A5A5A', light: '#2B91AF', hc: Color.white }, nls.localize('editorLineNumbers', 'Color of editor line numbers.'));
export const editorActiveLineNumber = registerColor('editorActiveLineNumber.foreground', { dark: null, light: null, hc: null }, nls.localize('editorActiveLineNumber', 'Color of editor active line number'));
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking if we should have this feature by default, by assigining proper foreground for default themes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting to assign colors for shipped with vscode themes: [monokai, red, ...] or default values for all themes? Assigning a default for all themes will look weird for a lot of themes: For instance, if I assign some gray color for all dark themes as default and a theme has a blue(or some other distant from gray) it would really stand out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @usernamehw to use 'null' as default here. That way we don't break anyone.

Copy link
Member

Choose a reason for hiding this comment

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

I'm convinced that null is a good value.

@rebornix rebornix added this to the February 2018 milestone Jan 30, 2018
@usernamehw
Copy link
Contributor Author

usernamehw commented Feb 12, 2018

Colors for default themes are set now...

@rebornix
Copy link
Member

@usernamehw sorry for the back-and-forth on whether we should have active line number support in default themes, can we revert the last commit and keep default active line number to null. Then we can merge this PR, thanks!

@usernamehw
Copy link
Contributor Author

usernamehw commented Feb 12, 2018

@rebornix, The default value is still null. I changed the color only for themes shipped with vscode.
But I can remove it if you want.

@rebornix rebornix merged commit 9af08ba into microsoft:master Feb 23, 2018
@rebornix
Copy link
Member

@usernamehw thanks for your contribution again ;) Active line number works extremely well for dark themes or themes which are already using active line background, so I'd like to cherry-pick some of your second commit (those active line number colors), hope you don't mind.

@usernamehw
Copy link
Contributor Author

Sure, it was the whole point of adding them.

@usernamehw usernamehw deleted the theming_active_line_number branch February 23, 2018 20:47
@github-actions github-actions bot locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[themes] Allow different color in line number
3 participants