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

Have command decorations use same red as scm decorations #143205

Closed
Tyriar opened this issue Feb 16, 2022 · 14 comments
Closed

Have command decorations use same red as scm decorations #143205

Tyriar opened this issue Feb 16, 2022 · 14 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders terminal-shell-integration Shell integration, command decorations, etc. verified Verification succeeded
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Feb 16, 2022

image

@Tyriar Tyriar added polish Cleanup and polish issue terminal-command-decoration labels Feb 16, 2022
@Tyriar Tyriar added this to the February 2022 milestone Feb 16, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Feb 16, 2022

Actually the default is right, it's just really dark by default on Dark+. @misolori we should probably tweak for contrast?

image

@Tyriar
Copy link
Member Author

Tyriar commented Feb 16, 2022

Also should we fallback to SCM colors so themes get the defaults if they're already set, or is that weird having terminal success/fail depend on scm added/deleted. It would be nice if we had some core colors for these sorts of common things so we could depend on platform/base instead of editor.

@meganrogge
Copy link
Contributor

Is there some other styling impacting how it looks in the editor vs the terminal? The color looks quite different.

Screen Shot 2022-02-16 at 10 19 54 AM

@Tyriar
Copy link
Member Author

Tyriar commented Feb 16, 2022

This is what I see in Dark+/Insiders:

image

@meganrogge
Copy link
Contributor

Oh I'm using my custom theme

@miguelsolorio
Copy link
Contributor

The SCM colors are being tweaked as part of #142809 I'd just make sure you are inheriting the color token and it should be updated automatically

@meganrogge
Copy link
Contributor

I think we're statically doing it at the moment - couldn't find where that code is located

@meganrogge
Copy link
Contributor

think I found it

@miguelsolorio
Copy link
Contributor

miguelsolorio commented Feb 16, 2022

Those are stored here:

export const editorGutterModifiedBackground = registerColor('editorGutter.modifiedBackground', {
dark: new Color(new RGBA(12, 125, 157)),
light: new Color(new RGBA(102, 175, 224)),
hc: new Color(new RGBA(0, 155, 249))
}, nls.localize('editorGutterModifiedBackground', "Editor gutter background color for lines that are modified."));
export const editorGutterAddedBackground = registerColor('editorGutter.addedBackground', {
dark: new Color(new RGBA(88, 124, 12)),
light: new Color(new RGBA(129, 184, 139)),
hc: new Color(new RGBA(51, 171, 78))
}, nls.localize('editorGutterAddedBackground', "Editor gutter background color for lines that are added."));
export const editorGutterDeletedBackground = registerColor('editorGutter.deletedBackground', {
dark: new Color(new RGBA(148, 21, 27)),
light: new Color(new RGBA(202, 75, 81)),
hc: new Color(new RGBA(252, 93, 109))
}, nls.localize('editorGutterDeletedBackground', "Editor gutter background color for lines that are deleted."));

And an example of how it's used in notebooks:

const modifiedBackground = theme.getColor(editorGutterModifiedBackground);
if (modifiedBackground) {
collector.addRule(`
.monaco-workbench .notebookOverlay .monaco-list .monaco-list-row.code-cell-row.nb-cell-modified .cell-focus-indicator {
background-color: ${modifiedBackground} !important;
}
.monaco-workbench .notebookOverlay .monaco-list .monaco-list-row.markdown-cell-row.nb-cell-modified {
background-color: ${modifiedBackground} !important;
}`);
}

@meganrogge
Copy link
Contributor

We should also decide if we want to show the "skipped" decoration, which color to use. Currently we use

export const TERMINAL_COMMAND_DECORATION_SKIPPED_BACKGROUND_COLOR = registerColor('terminalCommandDecoration.skippedBackground', {
light: '#00000040',
dark: '#ffffff40',
hc: '#ffffff80'
}, nls.localize('terminalCommandDecoration.skippedBackground', 'The terminal command decoration background color when the command was skipped (undefined exit code).'));

@Tyriar
Copy link
Member Author

Tyriar commented Feb 16, 2022

@meganrogge oh we still want new terminal theme keys, just to fallback to the gutter colors, like this:

export const TERMINAL_BORDER_COLOR = registerColor('terminal.border', {
dark: PANEL_BORDER,
light: PANEL_BORDER,
hc: PANEL_BORDER
}, nls.localize('terminal.border', 'The color of the border that separates split panes within the terminal. This defaults to panel.border.'));

@Tyriar Tyriar reopened this Feb 16, 2022
@meganrogge meganrogge reopened this Feb 17, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Feb 17, 2022

WRT the layer breaker, let's copy the gutter colors directly into the new theme colors instead of referencing them as we want theme authors to have control over what they are. The long term fix here is having a set of base theme colors we can depend upon in common

@Tyriar
Copy link
Member Author

Tyriar commented Feb 23, 2022

We want the following:

  • terminalCommandDecoration.defaultBackground (grey)
  • terminalCommandDecoration.successBackground (blue)
  • terminalCommandDecoration.errorBackground (red)

@Tyriar Tyriar closed this as completed in 3c1b455 Feb 23, 2022
@Tyriar Tyriar added bug Issue identified by VS Code Team member as probable bug and removed polish Cleanup and polish issue labels Feb 23, 2022
@Tyriar
Copy link
Member Author

Tyriar commented Feb 23, 2022

To verify ensure these theme keys work as expected:

terminalCommandDecoration.defaultBackground (grey)
terminalCommandDecoration.successBackground (blue)
terminalCommandDecoration.errorBackground (red)

@rzhao271 rzhao271 added the verified Verification succeeded label Feb 24, 2022
@Tyriar Tyriar added the terminal-shell-integration Shell integration, command decorations, etc. label Feb 28, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug insiders-released Patch has been released in VS Code Insiders terminal-shell-integration Shell integration, command decorations, etc. verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants
@Tyriar @rzhao271 @meganrogge @miguelsolorio and others