Skip to content

theme custom menus#54003

Merged
bpasero merged 6 commits intomasterfrom
sbatten/menuBarThemes
Jul 12, 2018
Merged

theme custom menus#54003
bpasero merged 6 commits intomasterfrom
sbatten/menuBarThemes

Conversation

@sbatten
Copy link
Copy Markdown
Member

@sbatten sbatten commented Jul 11, 2018

ref #52914

incomplete, theming the menubar (done) and menus (not done)

@sbatten sbatten requested a review from bpasero July 11, 2018 01:34
@bpasero bpasero self-assigned this Jul 11, 2018
@bpasero bpasero added this to the July 2018 milestone Jul 11, 2018
Copy link
Copy Markdown
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbatten I am not sure every color works as expected, here is some individual feedback:

  • menubar.inactiveMenuForeground: this color is not doing anything for me and I would rename it to menu.inactiveForeground. Once it works we should also add a corresponding menu.inactiveBackground
  • menubar.menuActiveBorder: I would rename this to menu.activeBorder and apply it for any active menu item including the menu bar
  • menubar.activeMenuBackground: I would rename this to menu.activeBackground and apply it for any active menu item including the menu bar
  • menubar.activeMenuForeground: I would rename this to menu.activeForeground and apply it for any active menu item including the menu bar

On top of that I think we also need these colors:

  • menu.background: overall background color of the menu (maybe only when expanded)
  • menu.foreground: overall foreground color of the menu
  • menu.hoverForeground and menu.hoverBackground will probably come up as well at one point

@sbatten sbatten force-pushed the sbatten/menuBarThemes branch from 6515699 to c4d35bd Compare July 12, 2018 00:44
@sbatten
Copy link
Copy Markdown
Member Author

sbatten commented Jul 12, 2018

@bpasero I've updated to be complete now. I've updated the names to be a bit less verbose in response to your feedback, but I've settled on a different set of keys. This is because the menubar needs to be separate from the menus themselves as it needs to blend with the titlebar. Due to this, I only allow theming of the menubar's selected item. The rest is decided by the titlebar.

Another note. I theme the menus in the menubarPart instead of attaching a styler to the menu for simplicity. If I understand correctly, exposing the colors/styling on the menu widget would require quite a bit more code since the menu composes an actionbar and thus we would also need to expose it there. Further, anywhere menus are created, the styler would have to be applied which includes the menubar, the context menus, and dropdown actions, at least.

@sbatten sbatten dismissed bpasero’s stale review July 12, 2018 07:00

changes made

@bpasero
Copy link
Copy Markdown
Member

bpasero commented Jul 12, 2018

To summarize, here are the new colors:

  • menubar.selectedBorder
  • menubar.selectedBackground
  • menubar.selectedForeground
  • menu.background
  • menu.foreground
  • menu.selectedBorder
  • menu.selectedBackground
  • menu.selectedForeground

The menubar.* colors only apply to this top level stripe:

image

Whereas the other colors apply to every menu, including the context menu.

I think the distinction between menubar and menu makes sense. I am not 100% sure about the word selected because this color applies under 2 conditions:

  • you hover over a menu item
  • you select a menu item with keyboard

In other areas we typically have a dedicated color for the case where you hover over an element vs. the active selected element. Do we want to distinguish between these 2 cases here as well? When I look at native menus on Windows there does not seem to be any difference between hovering and selection, so maybe its fine to keep this as one color, but then maybe it should not be called "selected" but rather "active"? Similar to these colors from tabs:

image

Adding @aeschli to this review to comment on the picked color keys.

Copy link
Copy Markdown
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great, I also suggest to make a pass over our built in themes and add the colors once we have decided on the final color keys.

@bpasero bpasero assigned sbatten and unassigned bpasero Jul 12, 2018
@aeschli
Copy link
Copy Markdown
Contributor

aeschli commented Jul 12, 2018

Names are good

@sbatten
Copy link
Copy Markdown
Member Author

sbatten commented Jul 12, 2018

@bpasero @aeschli yea, I want to make the behavior hover === keyboard selected so no distinction is necessary. that said, if names are good, please merge

@bpasero
Copy link
Copy Markdown
Member

bpasero commented Jul 12, 2018

@sbatten I renamed "selected" to "selection" and will merge.

Also do not forget to update our docs: https://code.visualstudio.com/docs/getstarted/theme-color-reference

@bpasero bpasero merged commit f62aad4 into master Jul 12, 2018
@bpasero bpasero deleted the sbatten/menuBarThemes branch July 12, 2018 08:03
@tweakimp
Copy link
Copy Markdown

What color is the divider between subgroups in the menu? Would it make sense to create a key for that, too?

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants