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

refactor menu theming to be cleaner and remove css colors #58568

Merged
merged 6 commits into from
Sep 18, 2018

Conversation

sbatten
Copy link
Member

@sbatten sbatten commented Sep 13, 2018

@sbatten sbatten added this to the September 2018 milestone Sep 13, 2018
@sbatten sbatten self-assigned this Sep 13, 2018
@bpasero
Copy link
Member

bpasero commented Sep 13, 2018

@sbatten please clean up the merge conflicts and bring to latest in master

Copy link
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.

The change makes sense to me, however introduces quite a bit of dynamic CSS. Ideally CSS is only used for complicated behaviour (e.g. when defining styles on :hover or :active). For simple widgets (e.g. the Button) we just try to set the style on the element directly. This would mean, instead of adding dynamic CSS, the Menu would have to just set the style on each menu item. But since the menu is probably never visible when the theme changes, it would probably even be fine to not update the colors dynamically but just using them when the menu is created again. Not sure how easy that is given the action items seem to be very generic, but wanted to bring it up.

@bpasero
Copy link
Member

bpasero commented Sep 14, 2018

@sbatten when you do these changes please cross check with the standalone editor as well that things are fine.

Copy link
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.

Much better 👍 .

There is one color usage left in .context-view.monaco-menu-container: background-color: white. not sure if that one should also be externalized?

@sbatten
Copy link
Member Author

sbatten commented Sep 18, 2018

@bpasero don't think that one is supposed to be there anymore, removed it

@sbatten sbatten merged commit c7cff38 into microsoft:master Sep 18, 2018
@sbatten sbatten deleted the sbatten/removeCssColors branch September 18, 2018 18:02
@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.

Do not hardcode border color for monaco-menu-container in HC theme
2 participants