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

Light HC theme missing watermark and keybindings hints #143765

Closed
roblourens opened this issue Feb 23, 2022 · 6 comments
Closed

Light HC theme missing watermark and keybindings hints #143765

roblourens opened this issue Feb 23, 2022 · 6 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug verified Verification succeeded
Milestone

Comments

@roblourens
Copy link
Member

Testing #143425

  • Close all editors
  • The logo watermark and keyboard shortcuts are missing, this is what it looks like in HC dark:

image

@daviddossett
Copy link
Contributor

daviddossett commented Feb 23, 2022

Dug into this a bit. It looks like there are two issues here:

First, we render different versions of the letterpress SVG depending on the theme type.

const letterpress = `./media/letterpress${theme.type === 'dark' ? '-dark' : theme.type === 'hc' ? '-hc' : ''}.svg`;

Second, our theme types are limited to dark, light, and hc.

export enum ColorScheme {
DARK = 'dark',
LIGHT = 'light',
HIGH_CONTRAST = 'hc'
}

@aeschli what do you think about splitting the hc types into something like hc-dark and hc-light? I can see how this could create problems elsewhere since we probably rely on that broad hc type for all sorts of overrides.

Another option could be to rely on on a different field like theme.baseTheme, although that has other drawbacks.

cc @isidorn @misolori for awareness on the type splitting question above.

@daviddossett daviddossett added the bug Issue identified by VS Code Team member as probable bug label Feb 23, 2022
@daviddossett
Copy link
Contributor

In the meantime I think can get by with using token vars e.g. fill:var(--vscode-foreground); in place of the hardcoded fill:white; in letterpress-hc.svg.

@daviddossett
Copy link
Contributor

daviddossett commented Feb 23, 2022

Fixed missing keybinding hints and updated the HC letterpress color to use a fixed hex value since it doesn't look like we can dynamically update the SVG's theme vars when the theme is changed.

CleanShot 2022-02-23 at 13 37 58@2x

CleanShot 2022-02-23 at 13 40 13@2x

@miguelsolorio
Copy link
Contributor

@daviddossett don't forget the same file on distro for the branded icon versions if you haven't already gotten to it

@daviddossett
Copy link
Contributor

Closing as fixed. Will create a separate issue for the theme type discussion.

@isidorn
Copy link
Contributor

isidorn commented Feb 24, 2022

@daviddossett please ping me on that other issue. I think it makes hc-light, and hc-dark thought it might be quite some debt work. @aeschli would know best I think.

@daviddossett daviddossett added this to the March 2022 milestone Mar 8, 2022
@roblourens roblourens added the verified Verification succeeded label Mar 23, 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 verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants