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

feat(theme): add contrast opacity values for all color types and hues #8872

Closed

Conversation

clshortfuse
Copy link
Contributor

@clshortfuse clshortfuse commented Jun 27, 2016

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

@clshortfuse clshortfuse added ui: theme needs: review This PR is waiting on review from the team labels Jun 27, 2016
@@ -689,6 +717,45 @@ function generateAllThemes($injector, $mdTheming) {
delete palette.contrastStrongLightColors;
delete palette.contrastDarkColors;

function getContrastType(hueName) {
if (defaultContrast === 'light') {
if (darkColors.indexOf(hueName) > -1) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inequality check is probably faster mathematically than "greater than" check. Updated.

@clshortfuse clshortfuse force-pushed the wip/mdTheme-contrastOpacities branch from 7b2188d to 46b716f Compare June 27, 2016 22:17
}
return 'light';
}
return 'dark';
Copy link
Member

Choose a reason for hiding this comment

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

Can't all of this be merged into one?

Copy link
Contributor Author

@clshortfuse clshortfuse Aug 2, 2016

Choose a reason for hiding this comment

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

I'm not sure I can refactor it much more:

Current:

if (a) {
  if (b) return 3;
  if (c) return 2;
  return 1;
}
if (d) {
  if (c) return 2;
  return 1;
}
return 3;

Into this:

if (a) {
  if (b) return 3;
  if (c) return 2;
  return 1;
}
if (!d) return 3;
if (c) return 2;
return 1;

then this:

if (a) {
  if (b) return 3;
  if (c) return 2;
} else {
  if (!d) return 3;
  if (c) return 2;
}
return 1;

then this:

if (a) {
  if (b) return 3;
} else {
  if (!d) return 3;
}
if (c) return 2;
return 1;

finally this:

if (a) {
  if (b) return 3;
} else if (!d) {
  return 3;
}
if (c) return 2;
return 1;

Edit: One more:

if (a ? b : !d) return 3;
if (c) return 2;
return 1;

@clshortfuse
Copy link
Contributor Author

I think we need to come up with a better solution for legacy support. Maybe if foregroundPalette isn't user generated set, we should use background-contrast instead

@clshortfuse clshortfuse added needs: work and removed needs: review This PR is waiting on review from the team labels Jun 29, 2016
@clshortfuse clshortfuse added the needs: team discussion This issue requires a decision from the team before moving forward. label Jun 30, 2016
@clshortfuse clshortfuse force-pushed the wip/mdTheme-contrastOpacities branch 2 times, most recently from 4946168 to 0f05fb3 Compare July 8, 2016 21:43
@clshortfuse
Copy link
Contributor Author

Ugh, apparently it pushed everything i wanted to revert over my old code. Now I'm on the hunt to see how I can undo it.

@clshortfuse clshortfuse closed this Jul 8, 2016
@clshortfuse clshortfuse force-pushed the wip/mdTheme-contrastOpacities branch from 0f05fb3 to 7f01138 Compare July 8, 2016 22:13
@clshortfuse clshortfuse reopened this Jul 8, 2016
@EladBezalel
Copy link
Member

Please squash your commits and have a commit message that follows our style guide

@clshortfuse clshortfuse added the needs: unit tests This PR needs unit tests to cover the changes being proposed label Jul 9, 2016
@clshortfuse clshortfuse force-pushed the wip/mdTheme-contrastOpacities branch 3 times, most recently from f12e15c to 4a22b01 Compare July 9, 2016 19:11
@clshortfuse clshortfuse added needs: review This PR is waiting on review from the team and removed needs: team discussion This issue requires a decision from the team before moving forward. needs: unit tests This PR needs unit tests to cover the changes being proposed needs: work labels Jul 9, 2016
@clshortfuse
Copy link
Contributor Author

@EladBezalel okay, I've reworked it and added some tests. I don't think there should any possible behavioral change. Stylistically, we'll be fixing incorrect opacities that are no longer part of spec (87% opacity white).

@@ -16,8 +16,7 @@ angular.module('material.core.theming.palette', [])
'A400': '#ff1744',
'A700': '#d50000',
'contrastDefaultColor': 'light',
'contrastDarkColors': '50 100 200 300 A100',
'contrastStrongLightColors': '400 500 600 700 A200 A400 A700'
Copy link
Member

Choose a reason for hiding this comment

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

Why contrastStrongLightColors? there are colors that the white or black doesn't have 87% opacity.

@clshortfuse clshortfuse force-pushed the wip/mdTheme-contrastOpacities branch 2 times, most recently from 1c0676d to e96e127 Compare July 10, 2016 10:53
@clshortfuse
Copy link
Contributor Author

Rebased. Would be nice to have this merged to fix some a11y issues

@ThomasBurleson ThomasBurleson modified the milestone: 1.2.0 Jan 1, 2017
@Splaktar Splaktar removed the needs: review This PR is waiting on review from the team label Nov 21, 2017
@Splaktar Splaktar added P0: critical Critical issues that must be addressed immediately. g3: reported The issue was reported by an internal or external product team. labels Nov 21, 2017
@Splaktar
Copy link
Contributor

Fixed the Travis issue. I'd like to run this through presubmit in the next batch.

@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 May 4, 2018
@Splaktar Splaktar changed the title feat(mdTheme) add contrast opacity values for all color types and hues feat(theme): add contrast opacity values for all color types and hues May 4, 2018
@Splaktar Splaktar requested review from Splaktar and removed request for topherfangio May 4, 2018 00:43
@Splaktar Splaktar self-assigned this May 4, 2018
@Splaktar Splaktar added the a11y This issue is related to accessibility label May 4, 2018
@Splaktar Splaktar modified the milestones: Future, 1.1.10 May 4, 2018
@Splaktar Splaktar added the needs: manual testing This issue or PR needs to have some manual testing and verification done label May 4, 2018
@Splaktar
Copy link
Contributor

Splaktar commented May 4, 2018

Can you please rebase this?

I'll try to do some manual testing and see if we can possibly get this in soon.

@Splaktar Splaktar added P1: urgent Urgent issues that should be addressed in the next minor or patch release. and removed P0: critical Critical issues that must be addressed immediately. labels May 4, 2018
@clshortfuse
Copy link
Contributor Author

@Splaktar I'll try to do it this weekend.

@Splaktar Splaktar modified the milestones: 1.1.10, 1.1.11 Jun 19, 2018
@Splaktar Splaktar removed their request for review July 20, 2018 11:20
@Splaktar
Copy link
Contributor

Going to close this stale PR and continue in #11376. @clshortfuse can you please comment in #11376 to confirm that it is OK to use your commits?

@Splaktar Splaktar closed this Jul 20, 2018
@Splaktar Splaktar removed this from the 1.1.11 milestone Jul 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y This issue is related to accessibility cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ g3: reported The issue was reported by an internal or external product team. needs: manual testing This issue or PR needs to have some manual testing and verification done needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved P1: urgent Urgent issues that should be addressed in the next minor or patch release. ui: theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants