feat(theming): add ability to specify hues as options to defineTheme #11428

xyng · 2018-08-27T09:54:45Z

extend test of $mdTheming to include one palette with and one without hue.
add docs regarding new paletteHues options for defineTheme method of $mdTheming.

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added or this is not a bug fix / enhancement
Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Enhancement
[x] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Currently, there is no possibility to apply hue-options to palettes when using defineTheme. This is described thoroughly in the following issue and Google Group.
https://groups.google.com/forum/#!topic/ngmaterial/d-D3MNFl6wg
Issue Number: #10335

What is the new behavior?

paletteHuesoptions have been added to the options object. These are completely optional.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I opted to add an option for each palette instead of changing each palette option to be either of type string or object, as that would be more likely to break or be confusing.
However I'm willing to implement it that way if you desire.

googlebot · 2018-08-27T09:54:49Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

xyng · 2018-08-27T09:58:38Z

We signed the cla. Google Group is: https://groups.google.com/forum/#!forum/interweber

Splaktar · 2018-08-29T01:22:10Z

Thank you for making your first contribution to AngularJS Material!

@xyng can you please verify that the email address associated with the git commit is a contributor in your Google Group and that it is also attached to your GitHub account.

If that all looks good, please respond with "I signed it!".

Splaktar

Thank you for submitting this! It's off to a good start. I just had a few comments.

When you update this branch, please make sure to amend your commit (rather than create a new commit) and then force push to your branch. This will allow you to avoid having to squash your commits later.

Splaktar · 2018-08-29T01:29:31Z

src/core/services/theming/theming.js

@@ -670,9 +670,13 @@ function ThemingProvider($mdColorPalette, $$mdMetaProvider) {
   * @param {object} options Theme definition options
   * Options are:<br/>
   * - `primary` - `{string}`: The name of the primary palette to use in the theme.<br/>
+   * - `primaryHues` - `{object} (Optional)`: Overwrite hues for primary palette.<br/>


Please use = at the end of the type to indicate that it is optional, i.e. {object=}.

Let's use "Override" instead of "Overwrite".

Splaktar · 2018-08-29T01:31:48Z

src/core/services/theming/theming.js

   * - `warn` - `{string}`: The name of the warn palette to use in the theme.<br/>
+   * - `warnHues` - `{object}`: Overwrite hues for warn palette.<br/>


Can you please add an example to the main $mdTheming docs where these new parameters are used?

Without that, it's not clear here what this object would consist of.

Splaktar · 2018-08-29T01:34:32Z

src/core/services/theming/theming.spec.js

    $mdTheming.defineTheme('test', {
      primary: 'red',
+      primaryHue: 300,


This should be primaryHues right? The value should also be an object?

Splaktar · 2018-08-29T01:35:23Z

src/core/services/theming/theming.spec.js

@@ -752,8 +752,10 @@ describe('$mdTheming service', function() {
  it('supports registering theme on the fly', inject(function ($mdTheming) {
    expect($mdTheming.THEMES.hasOwnProperty('test')).toBeFalsy();

+    // optional <color>Hue option for each color, as in normal theme definition.


After you make the other updates, please clarify this comment or remove it.

Splaktar · 2018-08-29T01:37:52Z

src/core/services/theming/theming.spec.js

    $mdTheming.defineTheme('test', {
      primary: 'red',
+      primaryHue: 300,


We should probably have a test that verifies that the correct hue was defined/used.

I have little experience with writing tests. I'll look into it and see what I can do.

xyng · 2018-08-29T09:29:38Z

src/core/services/theming/theming.js

@@ -585,6 +585,9 @@ function ThemingProvider($mdColorPalette, $$mdMetaProvider) {
   *
   *       $mdTheming.defineTheme('myTheme', {
   *         primary: 'blue',
+   *         primaryHues: {


I'm not sure if this doc makes things clear. The setPalette function seems to have no docs itself.
Any suggestions?

Here are the default hues that can be overridden and the valid keys in those objects:

Light mode hue defaults

'accent': { 'default': 'A200', 'hue-1': 'A100', 'hue-2': 'A400', 'hue-3': 'A700' }, 'background': { 'default': '50', 'hue-1': 'A100', 'hue-2': '100', 'hue-3': '300' }

Dark mode hue defaults

'background': { 'default': 'A400', 'hue-1': '800', 'hue-2': '900', 'hue-3': 'A200' }

Here are the default hue values for any other palette that doesn't specify its hues. We should link to the docs that describe this via ng-href="Theming/03_configuring_a_theme"

{ 'default': '500', 'hue-1': '300', 'hue-2': '800', 'hue-3': 'A100' }

Here are the valid hue values (we should mention these (not list them) and point to the same Configuring a theme doc mentioned above)

'50', '100', '200', '300', '400', '500', '600', '700', '800', '900', 'A100', 'A200', 'A400', 'A700'

I think that we should give an example that defines all of these hues for each of primary, accent, and warn. We should have one of the examples (perhaps warn) that only overrides 1 or 2 hues along with a mention about how these hues are optional. Then as mentioned above, we should link to the docs on configuring a theme as part of a brief description about the light/dark mode default hues (accent and background) and the default hues for all other palettes.

Thank you very much for this thorough information. I really appreciate your feedback.
I tried to implement it as good as possible - though I'm not the best at writing docs.

Sadly, I'm running out of time for this. So if you should have other suggestions regarding documentation I'd like to ask you to make these changes directly, if that is possible.

Unfortunately, it's non-trivial for me to make changes to your PR, but I do think that this is about ready to wrap up. Thank you for your cooperation.

xyng · 2018-08-29T09:54:37Z

src/core/services/theming/theming.spec.js

      warn: 'yellow'
    });

    expect($mdTheming.THEMES.hasOwnProperty('test')).toBeTruthy();
+    expect($mdTheming.THEMES.test.colors.primary.hues.default).toBe('300');


I suppose this should be enough to test the hue overrides. Suggestions?

The assertion looks like it will work.

However, it would be helpful if, instead of changing this test, you could add a new test (it(...)) that was similar but set and verified each of the different xHues fields.

I did a new test case, following your suggestion. I agree that this is a better way of testing it.

Splaktar · 2018-08-30T19:26:14Z

Hopefully those comments were helpful. Please don't hesitate to ask for clarification or any other information that you may need.

Thank you for your contributions!

googlebot · 2018-08-31T09:12:19Z

CLAs look good, thanks!

Splaktar · 2018-08-31T19:13:00Z

src/core/services/theming/theming.js

@@ -574,6 +574,8 @@ function ThemingProvider($mdColorPalette, $$mdMetaProvider) {
   * @description
   * Service that makes an element apply theming related <b>classes</b> to itself.
   *
+   * For more Information on the `xHues` objects, their default values, as well as valid hue values, please visit <a ng-href="Theming/03_configuring_a_theme#specifying-custom-hues-for-color-intentions">the custom hues section of Configuring a Theme</a>.


OK, out of respect for your limited time, I will hold off on some feedback, but I do want to fix one thing in this line.

Can you please change Information on the xHues objects to information on the hue objects?

* extend test of `$mdTheming` to include one palette with and one without hue. * add docs regarding new `paletteHues` option for `defineTheme` method of `$mdTheming`. Fixes angular#10335

Splaktar

LGTM

…ngular#11428)

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Aug 27, 2018

Splaktar self-assigned this Aug 29, 2018

Splaktar self-requested a review August 29, 2018 01:16

Splaktar suggested changes Aug 29, 2018

View reviewed changes

Splaktar added this to the 1.1.11 milestone Aug 29, 2018

xyng force-pushed the wip/feat-defineTheme-hues branch from e0e9945 to 03d5c1b Compare August 29, 2018 09:27

xyng commented Aug 29, 2018

View reviewed changes

xyng force-pushed the wip/feat-defineTheme-hues branch from 03d5c1b to abefd6c Compare August 29, 2018 09:53

xyng commented Aug 29, 2018

View reviewed changes

xyng force-pushed the wip/feat-defineTheme-hues branch from abefd6c to 50badc2 Compare August 31, 2018 09:12

googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Aug 31, 2018

xyng force-pushed the wip/feat-defineTheme-hues branch from 50badc2 to a14e7f3 Compare August 31, 2018 09:19

Splaktar reviewed Aug 31, 2018

View reviewed changes

Splaktar removed the needs: unit tests This PR needs unit tests to cover the changes being proposed label Aug 31, 2018

feat(theming): add ability to specify hues as options to defineTheme

21e6268

* extend test of `$mdTheming` to include one palette with and one without hue. * add docs regarding new `paletteHues` option for `defineTheme` method of `$mdTheming`. Fixes angular#10335

xyng force-pushed the wip/feat-defineTheme-hues branch from a14e7f3 to 21e6268 Compare September 3, 2018 06:06

Splaktar approved these changes Sep 4, 2018

View reviewed changes

Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer labels Sep 4, 2018

Splaktar removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Sep 4, 2018

Splaktar assigned andrewseguin and josephperrott and unassigned andrewseguin Sep 4, 2018

josephperrott merged commit f776bf7 into angular:master Sep 4, 2018

nobitagit pushed a commit to nobitagit/material that referenced this pull request Sep 24, 2018

feat(theming): add ability to specify hues as options to defineTheme (a…

96e2716

…ngular#11428)

marosoft pushed a commit to marosoft/material that referenced this pull request Nov 11, 2018

feat(theming): add ability to specify hues as options to defineTheme (a…

43f0b99

…ngular#11428)

		* - `warn` - `{string}`: The name of the warn palette to use in the theme.<br/>
		* - `warnHues` - `{object}`: Overwrite hues for warn palette.<br/>

feat(theming): add ability to specify hues as options to defineTheme #11428

feat(theming): add ability to specify hues as options to defineTheme #11428

Uh oh!

Conversation

xyng commented Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented Aug 27, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

xyng commented Aug 27, 2018

Uh oh!

Splaktar commented Aug 29, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Aug 30, 2018

Uh oh!

googlebot commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xyng commented Aug 27, 2018 •

edited

Loading

Splaktar Aug 30, 2018 •

edited

Loading