Selectbox theme additions Fixes #25965, #35246, #25700, #21193 #37533
Conversation
cleidigh
changed the title
|
@cleidigh good progress. Colors
Accessibility:
|
|
Addressed all comments, but a couple of questions/comments. Theme colors:
Hover background behavior:
Abyss
Dark (VSC)
Dark+ (default dark)
High Contrast
Kimbie Dark
Light (VSC)
Light (default light)
Monokai
Monokai Dimmed
Quiet Light
Red
Solarized Light
Solarized Dark
Tomorrow Night Blue
Accessibility/Behaviors:
Saludos |
There was a problem hiding this comment.
@cleidigh gave a bit of feedback on the change.
I think to make this PR easier to review I suggest to split the native select box and the custom one into one file each and use it depending on the OS.
I also question why we have to introduce so many new colors. I think all colors we need are already there via the list/tree colors and should be reused so that all our themes do not have to adopt this.
I still see some bugs:
- hovering over an item that is selected should not remove that selection color
- the dropdown cannot be used from the debug toolbar. you can reproduce by launching multiple programs from one window:
- you cannot click on the scrollbar to scroll, it will actually close the dropdown
| @@ -430,9 +431,10 @@ export class SwitchTerminalInstanceActionItem extends SelectActionItem { | |||
| constructor( | |||
| action: IAction, | |||
| @ITerminalService private terminalService: ITerminalService, | |||
| @IThemeService themeService: IThemeService | |||
| @IThemeService themeService: IThemeService, | |||
| @IContextViewService private contextViewService: IContextViewService | |||
There was a problem hiding this comment.
@cleidigh remove private here to fix the warning
| @@ -108,9 +109,10 @@ export class SwitchOutputActionItem extends SelectActionItem { | |||
| constructor( | |||
| action: IAction, | |||
| @IOutputService private outputService: IOutputService, | |||
| @IThemeService themeService: IThemeService | |||
| @IThemeService themeService: IThemeService, | |||
| @IContextViewService private contextViewService: IContextViewService | |||
There was a problem hiding this comment.
@cleidigh remove private here to fix the warning
| @@ -184,9 +186,10 @@ export class FocusProcessActionItem extends SelectActionItem { | |||
| constructor( | |||
| action: IAction, | |||
| @IDebugService private debugService: IDebugService, | |||
| @IThemeService themeService: IThemeService | |||
| @IThemeService themeService: IThemeService, | |||
| @IContextViewService private contextViewService: IContextViewService | |||
There was a problem hiding this comment.
@cleidigh remove private here to fix the warning
| @@ -40,10 +41,11 @@ export class StartDebugActionItem implements IActionItem { | |||
| @IDebugService private debugService: IDebugService, | |||
| @IThemeService private themeService: IThemeService, | |||
| @IConfigurationService private configurationService: IConfigurationService, | |||
| @ICommandService private commandService: ICommandService | |||
| @ICommandService private commandService: ICommandService, | |||
| @IContextViewService private contextViewService: IContextViewService, | |||
There was a problem hiding this comment.
@cleidigh remove private here to fix the warning
There was a problem hiding this comment.
Done
Is this new warning? I don't think I saw this before.
There was a problem hiding this comment.
@cleidigh yes we added lots of more warning support recently by adopting TSLint 5
| @@ -183,6 +183,11 @@ export const inputValidationErrorBorder = registerColor('inputValidation.errorBo | |||
| export const selectBackground = registerColor('dropdown.background', { dark: '#3C3C3C', light: Color.white, hc: Color.black }, nls.localize('dropdownBackground', "Dropdown background.")); | |||
| export const selectForeground = registerColor('dropdown.foreground', { dark: '#F0F0F0', light: null, hc: Color.white }, nls.localize('dropdownForeground', "Dropdown foreground.")); | |||
| export const selectBorder = registerColor('dropdown.border', { dark: selectBackground, light: '#CECECE', hc: contrastBorder }, nls.localize('dropdownBorder', "Dropdown border.")); | |||
| export const selectOptionCheckedForeground = registerColor('dropdown.optionCheckedForeground', { dark: Color.white, light: Color.white, hc: null }, nls.localize('dropdownOptionCheckedForground', "Dropdown option checked foreground.")); | |||
There was a problem hiding this comment.
@cleidigh suggest to make the description more descriptive and use a different name than "option" which is very HTML specific. E.g. to align with existing colors we already have for the tree:
dropdown.optionCheckedForeground
- rename to
dropdown.selectionForeground - description:
Foreground color of the selected item in the dropdown.
dropdown.optionCheckedBackground
- rename to
dropdown.selectionBackground - description:
Background color of the selected item in the dropdown.
dropdown.optionHoverForeground
- rename to
dropdown.hoverForeground - description:
Foreground color of items in the dropdown when hovering over.
dropdown.optionHoverBackground
- rename to
dropdown.hoverBackground - description:
Background color of items in the dropdown when hovering over.
There was a problem hiding this comment.
@cleidigh actually thinking more about it, why do we have these colors at all? can we not just use the tree colors? since we are already using the same widget I see no much value in having different colors only for the dropdown....
There was a problem hiding this comment.
Using list colors.
| this.widthControlElement.className = 'option-text-width-control'; | ||
| dom.append(widthControlInnerDiv, this.widthControlElement); | ||
| // this.widthControDivElement = widthControlInnerDiv; | ||
| // TODO:cleidigh - find better width control |
There was a problem hiding this comment.
@cleidigh can you fix or remove this TODO?
| if (!this._useNativeSelect) { | ||
|
|
||
| if (this.styles.selectOptionCheckedBackground) { | ||
| content.push(`.monaco-shell .select-dropdown-container .selectbox-dropdown-list-container .monaco-list .monaco-list-row.focused:not(:hover) { background-color: ${this.styles.selectOptionCheckedBackground} !important; }`); |
There was a problem hiding this comment.
@cleidigh again please express the DOM hierarchy if possible, e.g. .select-dropdown-container .selectbox-dropdown-list-container => .select-dropdown-container > .selectbox-dropdown-list-container
| if (this.options[index].length > this.options[longest].length) { | ||
| longest = index; | ||
| } | ||
| }; |
|
|
||
| this.toDispose.push(dom.addDisposableListener(this.selectElement, dom.EventType.CLICK, () => { | ||
| this.showSelectDropDown(); | ||
| event.preventDefault(); |
| return elementWidth; | ||
| } | ||
|
|
||
| public cloneElementFont(source: HTMLElement, target: HTMLElement) { |
There was a problem hiding this comment.
@cleidigh not so happy:
- why is the method
public? - it uses
var, which we try to avoid (e.g. useletorconstinstead) - can the value be cached and reused instead of always asking for it?
|
@bpasero |
|
Selection color precedence over hover: I had understood the opposite before, my bad... Loss a focus clicking scrollbar: Had to detect clicks on scrollbar versus items. |
|
@cleidigh awesome. take your time, I think with the stuff addressed we can merge this in for December milestone. Splitting the custom part out of the native one makes it easier for me to review the change. Now it is very hard to understand the impact for the native implementation. I think stuff will be cleaner when the two implementations are separated. |
|
@bpasero For the class separation: I am looking into possible design patterns and want your input before I cut things up. Most basic:
Mixin Methods:
Preference or other option? |
|
@bpasero I have one other problem - I am trying to see the problem with the debug drop-down. |
|
@cleidigh yeah here is how to do it:
|
|
Cool I will try that. Any preference on the separation approach? |
|
@cleidigh well maybe the |
|
@bpasero |
|
@cleidigh I think it would work with an interface. basically you are delegating the calls to selectBox.ts into the implementation through the interface. So depending on which implementation is enabled it will reach either native or custom implementation. |
|
@bpasero |
|
@cleidigh maybe easier if you try to debug an extension because everything is setup for you there. Just:
You should get the debug toolbar with a dropdown. |
|
@bpasero |
|
@bpasero Everything should be pretty clean now, I now use all list colors and passed focusBorder directly into FYI: Several themes have little to no contrast for the list select and list hover. These include Monokai Hopefully it looks good to you... |
cleidigh
changed the title
|
Missed a couple things 👎
|
|
@cleidigh thanks for the work here 👍 |
|
@bpasero
I appreciate you working with me on this for all this time. Really learned a lot for future contributions ! |
|
@cleidigh ah yeah I did not see that the disposal thing was passed over. I think I still prefer the style where the disposing is passed on to the child element, that is what we typically do in other places. And yeah, the |
Selectbox theme additions Fixes #25965, #35246, #25700, #25619, #21193
I fixed all the issues I have found as well as addressing a lot of aesthetic issues.
Vertical margins addressed
Horizontal margins - controlled by ContextV - only flips when window edge hit
Could address this with optional margin requirements passed to ContextV (your call)
Cannot use CSS or forced resize because it conflicts with ContextV logic
Added VerticalScrollBar visibility option to listview - could also add horizontal
Tweaked margins for high contrast mode
DropDown is fully modal - could pass through other keyboard commands if desired
Re-Tested non-native select mode on : Windows 7, OS X, Debian
As first list/ContextV effort , make sure I didn't miss use anything !
See prior PR for more discussion #36193