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

fix: Add dark mode styling for nested menu items #1543

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

sfoslund
Copy link
Member

Details

Update styling for menu items such that they adhere to dark mode. This effects the hierarchy settings and event settings menus that were identified in #1528.

Motivation

Addresses issue #1528

Context

[Image grid description: Screenshots of each menu before and after the styling change in dark and light modes]

Before After
Hierarchy settings in light mode light-livesettings-before Light-livesettings-after
Hierarchy settings in dark mode dark-livesettings-before dark-livesettings-after
Event settings in light mode light-eventsettings-before light-eventsettings-after
Event settings in dark mode dark-eventsettings-before dark-eventsettings-after

Pull request checklist

@sfoslund sfoslund requested a review from a team as a code owner January 23, 2023 17:21
@DaveTryon
Copy link
Contributor

I'm not getting the dark theming on the settings buttons in either live mode:

image

or in events mode:

image

Is it possible that a modified file didn't get pushed?

@DaveTryon
Copy link
Contributor

We're close, but there's insufficient contrast between the menu hover color and the text hover color in the second tier of menus (see below)--maybe the second tier style should also update the text hover color?

image

image

@sfoslund
Copy link
Member Author

@DaveTryon I've fixed the color contrast issue but I am not able to repro the issue you mentioned in your first comment- can you please provide repro steps if you're still seeing this?

@DaveTryon
Copy link
Contributor

DaveTryon commented Feb 27, 2023

Thanks, @sfoslund! The problems I was having earlier seem to come from turning HC modes on and off while the app is running. I can still repro with some effort but it shouldn't be a blocker. I also found #1566 while testing these changes, but that's apparently an old bug unrelated to these changes.

One thought that might be very reasonably deferred--we now have something of a visual imbalance in how hover/selections are highlighted between the hierarchy control (highlight with contrast color) and the property control (highlight via bold text). Is it worth the effort to make them more consistent? We need to be mindful about not over-theming, since we also have context menus in non-themed dialogs.

@sfoslund sfoslund merged commit de118c3 into microsoft:main Feb 27, 2023
@sfoslund sfoslund deleted the FixStyle branch February 27, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants