Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(menu): support dark themes and non-grey backgrounds #9212

Closed

Conversation

clshortfuse
Copy link
Contributor

@clshortfuse clshortfuse commented Aug 2, 2016

Menu popups should always be lightly colored and its content
should use their respective contrast color and opactiy.

  • Set background color to background-100
  • Use contrast values for buttons, icons, and dividers

Fixes #9100

@clshortfuse clshortfuse added ui: theme Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. labels Aug 2, 2016
@clshortfuse
Copy link
Contributor Author

clshortfuse commented Aug 3, 2016

Blocked by PR #8872 #11376

@EladBezalel
Copy link
Member

@clshortfuse i'm not sure it's correct by the material design spec,
please take a look at #5744

@clshortfuse
Copy link
Contributor Author

clshortfuse commented Aug 15, 2016

@EladBezalel I see. Strange because that's not how Android's implementation of Material works.

Still, there is an issue because it's not using the right contrast values causing issues if you use anything other than gray. I'll update it to be lighter but still support changing backgroundPalette.

@clshortfuse clshortfuse force-pushed the menu-fixContrastColors branch 2 times, most recently from b0a5e5a to 25554da Compare August 16, 2016 01:32
@clshortfuse clshortfuse changed the title fix(menu): support dark theme fix(menu): support dark themes and non-grey backgrounds Aug 16, 2016
@ThomasBurleson ThomasBurleson added this to the 1.1.2 milestone Aug 22, 2016
@clshortfuse
Copy link
Contributor Author

clshortfuse commented Sep 7, 2016

Changed to hue-100 per spec. Now backgrounds properly render regardless of background palette.

Before: Before

After: After

Dark: Dark

@EladBezalel
Copy link
Member

@clshortfuse dropdown menus should always be white:
image

Add opacity keyword support (`secondary`, `icon`, `disabled`, `hint`, `divider`)
Deprecate documentation of `foreground-*` in favor of `background-default-contrast-*`
Allow `foregroundPallete` to override colors on default background and hues of equal contrast type
Menu popups should always be lightly colored and its content
should use their respective contrast color and opactiy.

 - Set background color to background-50
 - Use contrast values for buttons, icons, and dividers

Fixes angular#9100
@clshortfuse
Copy link
Contributor Author

@EladBezalel

I took a colorpicker against the dropdown menu and it's not white. It's #FAFAFA which is actually grey-50. I'm using background-100 in the current screenshots above, so background-50 should keep it all to spec. It'll partially tint based on background color if users change backgroundPalette.

What it looks like now:

Light:
screenshot 2016-09-08 at 2 14 55 pm

Dark:
screenshot 2016-09-08 at 2 12 24 pm

If we want to hardcode the colors (#FAFAFA), it'll create some complications if the user decides to change the foreground color and/or use a dark theme. Perhaps we can add support in mdTheme for referencing by palette name, for example {{grey-50-background-contrast}}. As far as I can tell, that would be the safest solution.

@clshortfuse clshortfuse force-pushed the menu-fixContrastColors branch from 25554da to aca037f Compare September 8, 2016 18:18
@ThomasBurleson ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 9, 2016
@clshortfuse clshortfuse added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Sep 11, 2016
@clshortfuse
Copy link
Contributor Author

@EladBezalel can you take a look at it now that I switched to background-50. Or wait for the other PR, not sure how we should approach reviewing this.

@EladBezalel
Copy link
Member

EladBezalel commented Sep 11, 2016

So i'm really not sure what we should assume about background colors..
I'm also not sure if even you have different background color you'd like background-50 and not #fafafa..

If we had a clear spec that said background-50 than I wouldn't be afraid it may break some people, maybe we want to postpone it to 1.2 ? i'm not really sure, it's a hard one!

@clshortfuse clshortfuse modified the milestones: 1.2.0, 1.1.2 Nov 16, 2016
@clshortfuse clshortfuse added Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. and removed Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. labels Dec 22, 2016
@Splaktar Splaktar added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Feb 7, 2018
@Splaktar Splaktar self-requested a review February 7, 2018 08:35
@Splaktar Splaktar modified the milestones: Future, 1.2.0 Feb 7, 2018
@Splaktar Splaktar added the P2: required Issues that must be fixed. label Feb 7, 2018
@Splaktar Splaktar assigned Splaktar and unassigned EladBezalel Aug 26, 2019
@Splaktar Splaktar removed this from the 1.2.0 milestone Aug 26, 2019
@Splaktar Splaktar removed the needs: review This PR is waiting on review from the team label Aug 26, 2019
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

This PR gives the following behavior (after rebasing, see this branch)
Screen Shot 2019-08-26 at 13 28 37

After we made a bunch of dark theme updates, 1.1.20 now gives this behavior
Screen Shot 2019-08-26 at 13 32 15

We're going to stick with the current behavior in 1.1.20 as it better aligns with our expectations of dark mode.

@Splaktar Splaktar closed this Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Blocked Progress on this issue is blocked. Primarily used for PRs that are blocked by presubmit feedback. needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved P2: required Issues that must be fixed. ui: theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

menu: assigning a color to backgroundPalette renders menu unreadable
5 participants