Custom colors clean up #1252
Custom colors clean up #1252
Conversation
per1234
added
topic: theme
There was a problem hiding this comment.
Ok, so there's a lot to analyze here...
Remove hard-coded colors using appropriate variables.
Seems we should and I don't see why we shouldn't. ✅
To change "arduino.output.background" and "arduino.output.foreground" with "terminal.background" and "terminal.foreground" (https://code.visualstudio.com/api/references/theme-color#integrated-terminal-colors). The values of these variables must be defined.
I don't think there is a reason why they should be different. At least not as for now. @91volt do you think we are going to have different colours for terminal and output in the immediate future? If not, I'd ✅
"arduino.branding.primary" used for the "Learn more" link in the Cloud Sketchbook, could be replaced by "textLink.foreground" (https://code.visualstudio.com/api/references/theme-color#text-colors). The values of the new variable must be defined.
I don't see specific pitfalls here. If we are planning to do crazy stuff with textLink we can just set arduino.branding.primary default to textLink.foreground. Unless really needed, let's complicate the things when it's needed. Not before. ✅ here as well for me
"arduino.branding.primary" used for the "Select board and port..." button in the board selector dropdown, could be replaced by "secondaryButton.foreground". This variable is already present and his values reflects those of the older one.
The arduino.branding.primary has no contextual meaning, hence secondaryButton.foreground it's probably better as it provides some semantic information. ✅ as long as it does not clash with other colors, but reading your comment I think it does not.
"arduino.branding.primary" used for the check icon in the "Select Board" dialog, can be replaced by "list.activeSelectionIconForeground" (https://code.visualstudio.com/api/references/theme-color#lists-and-trees). The values of the new variable must be defined.
I'm a little skeptical here, because we are using a different, yet similar, semantic name. It's also true that arduino.branding.primary has no meaning at all. So can we decide between these two options ❓
- use
list.activeSelectionIconForeground(but please comment in the code that using this color can be considered "improper" - add a new semantic "arduino" variable that defaults to
list.activeSelectionIconForegroundwhen it's not set in a theme
"arduino.branding.primary" used for the "Select Other Board & Port" string in the "Select Board" dialog, could be replaced by "editorWidget.foreground" (https://code.visualstudio.com/api/references/theme-color#editor-widget-colors). The values of the new variable must be defined.
This seems good to me ✅
"arduino.branding.secondary" used for the "NO PORTS DISCOVERED" message in the "Select Board" dialog, could be replaced by "editorWidget.foreground". We can use this variable also for the string "SEARCH BOARD" in the same dialog since "The Editor widget is shown in front of the editor content. Examples are the Find/Replace dialog, the suggestion widget, and the editor hover." (https://code.visualstudio.com/api/references/theme-color#editor-widget-colors).
Not sure I get this one. will the change you suggest have impacts on the current colors ❓
Since the colors of the tabs in the settings dialog are hard-coded, could be possible to use the tab-activeBackground, tab-activeBorder and tab-activeForeground variables (https://code.visualstudio.com/api/references/theme-color#editor-groups-tabs). The values of the new variables must be defined.
This seems quite straightforward to me, I'd say yes ✅
Regarding the board selector dropdown it could be possible to replace: "arduino.toolbar.dropdown.background" with "dropdown.background", "arduino.toolbar.dropdown.label" with "dropdown.foreground", "arduino.toolbar.dropdown.border" with "dropdown.border" (https://code.visualstudio.com/api/references/theme-color#dropdown-control). The values are already defined and are corresponding.
If they are already defined and corresponding, I'd say yes ✅ . In the future we might need to differentiate between the two, but let's keep the codebase simple as long as we can.
Regarding the active border of the board selector dropdown could be possible to use, as in Theia Blueprint, "focus.border" instead of "arduino.toolbar.dropdown.borderActive" (https://code.visualstudio.com/api/references/theme-color#base-colors).
I think it's ok, but does it mean changing the focus.border color in order to achieve the same result. If so, we need to double check it's not used (or if used does not break stuff). So questionmark ❓ here
Regarding the board selector drop-down when it's open we can consider the items inside as a list (for the drop-down menus of the Serial Monitor we use the list variables) and we could use these: https://code.visualstudio.com/api/references/theme-color#lists-and-trees, in particular: "list.hoverBackground" instead of "arduino.toolbar.dropdown.option.backgroundHover", "list.activeSelectionForeground" instead of "arduino.toolbar.dropdown.option.backgroundSelected", "list.activeSelectionIconForeground" instead of "arduino.toolbar.dropdown.iconSelected".
Similar comment as for the "Select Board" . You are proposing to use a similar, yet slightly different semantic meaning. we can probably set those you suggest as defaults, just for the sake of clarity. what do you think ❓
|
Thanks for you review @fstasi! Below I try to explain all the considerations I have made for the points that are not clear.
The variable So, yes, this change has an impact on current colors. The current state is the one shown in the attached image for the previous point. This is the suggested change:
and this is the requirement:
A custom value for
This is the trickiest part since I assumed to consider the open board selector drop-down menu as a list (like the Serial Monitor drop-down menu in the image from the previous point). For this point I'd wait for the opinion of @91volt. |
francescospissu
force-pushed
the
fspissu/themes-cleanup
branch
from
8d96fe2 to
1150a41
Compare
✅
✅
✅
✅ Works for me TASK FOR ME
🟡 -> I believe the identified token might be misleading, and we should address this using just one dialog title in dark gray as all other dialogs, it's redundant to keep both titles.
🔴 -> I think that our dialog shouldn't be confused with internal editor's widgets such as find and replace. We should have a contextual variable for arduino dialogs subtle foregrounds. Token: arduino.dialog.foreground.subtle.
✅ If they are not in usage works for me, and in case ->TASK FOR ME
🔴 Since the arduino toolbar and board selector are our own custom components I would keep them autonomous from other vscode tokens. -> But all these tokens you found are useful fallbacks for compatibility with other themes
🔴 -> Same as toolbar and dropdown I believe we should keep them autonomous and I would like to keep the granularity for each button, primary and secondary. -> Same as before, this are useful fallbacks. This is my point of view that takes into account the usage of the color tokens shared with the web editor in the future. |
Actually these variables are also used for the editor tabs:
Do you think we can use these for both purposes @91volt?
About this, my assumption was to consider the board selector drop-down menu as a "standard" drop-down menu which, when expanded, presents the items inside as a list. This is probably not correct from a design point of view, so it's possible to revert to the previous behavior using the custom variables:
This is also forced from my point of view in fact I didn't applied these changes. In this case the number of the custom colors will be reduced from 17 custom color entries to 15: New: Reverted: Already present: |
✅ Ok, thanks for highlighting that we are already using editorWidget tokens for dialogs.
✅ Yes it looks feasible since in our dark and light themes the IDE main area background color is the same - or very close - to the dialog bg. At a certain point we should replace the preferences panel, so we won't have anymore this kind of tabs navigation in the dialog, and this sounds the best way to avoid adding variables.
Yep I confirm would like to revert to the previous state of tokens here.
👌 Perfect |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…de into fspissu/themes-cleanup
|
Merging this PR the number of custom colors has been reduced to 12 items:
|
Motivation
The number of custom colors should be reduced because in the future it will be difficult to support VS Code themes while keeping all these extra colors.
Hard-coded colors must be removed using an appropriate variable.
Change description
The following is a proposed change taking into account these variables https://code.visualstudio.com/api/references/theme-color.
In particular:
Remove hard-coded colors using appropriate variables.
To change
"arduino.output.background"and"arduino.output.foreground"with"terminal.background"and"terminal.foreground"(https://code.visualstudio.com/api/references/theme-color#integrated-terminal-colors). The values of these variables must be defined."arduino.branding.primary"used for the "Learn more" link in the Cloud Sketchbook, could be replaced by"textLink.foreground"(https://code.visualstudio.com/api/references/theme-color#text-colors). The values of the new variable must be defined."arduino.branding.primary"used for the "Select board and port..." button in the board selector dropdown, could be replaced by"secondaryButton.foreground". This variable is already present and his values reflects those of the older one."arduino.branding.primary"used for the check icon in the "Select Board" dialog, can be replaced by"list.activeSelectionIconForeground"(https://code.visualstudio.com/api/references/theme-color#lists-and-trees). The values of the new variable must be defined."arduino.branding.primary"used for the "Select Other Board & Port" string in the "Select Board" dialog, could be replaced by"editorWidget.foreground"(https://code.visualstudio.com/api/references/theme-color#editor-widget-colors). The values of the new variable must be defined."arduino.branding.secondary"used for the "NO PORTS DISCOVERED" message in the "Select Board" dialog, could be replaced by"editorWidget.foreground". We can use this variable also for the string "SEARCH BOARD" in the same dialog since "The Editor widget is shown in front of the editor content. Examples are the Find/Replace dialog, the suggestion widget, and the editor hover." (https://code.visualstudio.com/api/references/theme-color#editor-widget-colors).Since the colors of the tabs in the settings dialog are hard-coded, could be possible to use the
tab-activeBackground,tab-activeBorderandtab-activeForegroundvariables (https://code.visualstudio.com/api/references/theme-color#editor-groups-tabs). The values of the new variables must be defined.Regarding the board selector dropdown it could be possible to replace:
"arduino.toolbar.dropdown.background"with"dropdown.background","arduino.toolbar.dropdown.label"with"dropdown.foreground","arduino.toolbar.dropdown.border"with"dropdown.border"(https://code.visualstudio.com/api/references/theme-color#dropdown-control). The values are already defined and are corresponding.Regarding the active border of the board selector dropdown could be possible to use, as in Theia Blueprint,
"focus.border"instead of"arduino.toolbar.dropdown.borderActive"(https://code.visualstudio.com/api/references/theme-color#base-colors).Regarding the board selector drop-down when it's open we can consider the items inside as a list (for the drop-down menus of the Serial Monitor we use the list variables) and we could use these: https://code.visualstudio.com/api/references/theme-color#lists-and-trees, in particular:
"list.hoverBackground"instead of"arduino.toolbar.dropdown.option.backgroundHover","list.activeSelectionForeground"instead of"arduino.toolbar.dropdown.option.backgroundSelected","list.activeSelectionIconForeground"instead of"arduino.toolbar.dropdown.iconSelected".Regarding toolbar buttons, it is difficult to find a solution since they are very custom elements. But I noticed that there is also a toolbar in Theia Blueprint:
For these items custom colors defined on Theia are used:
"mainToolbar.background","mainToolbar.foreground"(https://github.com/eclipse-theia/theia/blob/4b1220ac6602b794ff0369d01acd8b35f910e757/packages/core/src/browser/common-frontend-contribution.ts#L2209-L2223) and"toolbar.hoverBackground"defined in https://code.visualstudio.com/api/references/theme-color#action-colors.It could be possible to use the same variables used for those buttons.
With these changes (without using the variables for the toolbar icons) we reduce the custom colors only to:
'arduino.toolbar.button.background''arduino.toolbar.button.hoverBackground''arduino.toolbar.button.secondary.label''arduino.toolbar.button.secondary.hoverBackground''arduino.toolbar.toggleBackground'Other information
Closes #1226 and #1224.
Reviewer checklist