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

improve handling fallback of theme colors in terminal #57100

Merged
merged 3 commits into from Sep 11, 2018

Conversation

Projects
None yet
2 participants
@ParkourKarthik
Contributor

ParkourKarthik commented Aug 23, 2018

This closes #57038

@Tyriar
Yet to change defaults for TERMINAL_BORDER_COLOR.
I am not able to relate it to any appropriate editor colors (though there are editorWidgetBorder, editorHoverBorder, etc).

Should I consider hard-coding the defaults same as PANEL_BORDER?

export const PANEL_BORDER = registerColor('panel.border', {
dark: Color.fromHex('#808080').transparent(0.35),
light: Color.fromHex('#808080').transparent(0.35),
hc: contrastBorder
}, nls.localize('panelBorder', "Panel border color to separate the panel from the editor. Panels are shown below the editor area and contain views like output and integrated terminal."));

@Tyriar

@ParkourKarthik why can't PANEL_BORDER be imported like editorBackground?

We also want to remove the fallback code here as it's handled when the color is registered now:

const backgroundColor = theme.getColor(TERMINAL_BACKGROUND_COLOR) || theme.getColor(PANEL_BACKGROUND);
collector.addRule(`.monaco-workbench .panel.integrated-terminal .terminal-outer-container { background-color: ${backgroundColor ? backgroundColor.toString() : ''}; }`);
const borderColor = theme.getColor(TERMINAL_BORDER_COLOR) || theme.getColor(PANEL_BORDER);

@ParkourKarthik

This comment has been minimized.

Show comment
Hide comment
@ParkourKarthik

ParkourKarthik Aug 24, 2018

Contributor

Yep, we could import it. Wanted to make sure that it is the correct one. I'll remove the respective fallback code as well.

Contributor

ParkourKarthik commented Aug 24, 2018

Yep, we could import it. Wanted to make sure that it is the correct one. I'll remove the respective fallback code as well.

@ParkourKarthik

This comment has been minimized.

Show comment
Hide comment
@ParkourKarthik

ParkourKarthik Aug 26, 2018

Contributor

@Tyriar
Could you please review and close this PR.

Contributor

ParkourKarthik commented Aug 26, 2018

@Tyriar
Could you please review and close this PR.

@Tyriar Tyriar added this to the September 2018 milestone Aug 30, 2018

@Tyriar

Tyriar approved these changes Aug 30, 2018

Looks good, will merge after the release is branched off. Thanks!

@Tyriar Tyriar merged commit 5483440 into Microsoft:master Sep 11, 2018

1 of 2 checks passed

VS Code #20180905.79 failed
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment