feat(stepper): Create MAT_STEPPER_GLOBAL_OPTIONS InjectionToken #11457

arodr967 · 2018-05-22T18:26:10Z

Changes

Major

Create MAT_STEPPER_GLOBAL_OPTIONS InjectionToken to easily configure the stepper to either showError for the steps and/or useGuidelines to either use the existing logic or new logic to display the icons.
Create a state input for the step so that the developer can set their own state if necessary.
Refactor the _getIndicatorType logic to adhere to Material UI Guidelines if useGuidelines is set to true.
Add an error state! 🎉

~~### Breaking~~
~~- Removed completed as an input. However, the developer should still be able to override this attribute through the controller.~~

This PR will resolve the following opened issues:

Linear stepper shows edit icons instead of done icons Linear stepper shows edit icons instead of done icons #8997
[FR] Allow theming a step when invalid+dirty [FR] Allow theming a step when invalid+dirty #8950

Acknowledgments:

Big thanks to @zackarychapple for kick-starting this! 🙌

zackarychapple · 2018-05-22T18:34:12Z

cc/ @mmalerba, this is part one of the PR from our conversation on lifecycle hooks and state.

mmalerba · 2018-05-22T18:42:37Z

I think we need a design doc on this first, specifically focusing on how we make these changes in a way that causes as few breaking changes as possible

zackarychapple · 2018-05-22T19:35:45Z

@mmalerba I'll work with @arodr967 on the doc and post a link here.

ngbot · 2018-07-11T15:10:11Z

Hi @arodr967! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-07-11T15:10:30Z

Hi @arodr967! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-07-11T15:11:13Z

Hi @arodr967! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

zackarychapple · 2018-07-19T17:02:29Z

@mmalerba can you take a peek at this when you get a chance.

arodr967 · 2018-07-26T15:31:47Z

@mmalerba Any update on this? Is there anything that I need to do? Thanks!

mmalerba

@arodr967 sorry! I wrote some comments and forgot to send them

mmalerba · 2018-07-24T21:03:17Z

e2e/components/stepper-e2e.spec.ts

@@ -73,7 +73,7 @@ describe('stepper', () => {
      nextButton.get(0).click();

      expect(await element(by.css('mat-step-header[aria-selected="true"]')).getText())
-          .toBe('1\nFill out your name');
+          .toBe('create\nFill out your name');


Aren't these changes indicative of a breaking change of some kind?

@mmalerba

The completed and editable inputs have no breaking change, they will still work as before. (I need to add completed back as an Input).

hasError would create a breaking change; therefore, I will be adding a showError input which will allow devs to have the option to display the error or not and by default it will be set to false, instead of doing it automatically as I am now. So this would be an enhancement.

The changes in functionality are now in regards to the done and edit icons. This is because before it would always show the edit icon, regardless of the step being completed. And now, instead of showing the edit icon when it's not the current step, it's only showing the edit icon when it is the current step (which is why the tests were changed). If you think that we should also keep the way it was working before then I could add some type of Input which would allow the developer to either use the new functionality or the old one (old one would be set to the default).

Thoughts?

Rather than add @Input()s for these, another option is to create an injectable MAT_STEPPER_GLOBAL_OPTIONS. We have this for a few other components (e.g. mat-label: https://github.com/angular/material2/blob/d6fec3573cbc854d5bf67d920c5b0f8a6eef0c86/src/lib/core/label/label-options.ts). This way people could enable it for all steppers in their application simultaneously

Ok I am going to try that, thanks!

mmalerba · 2018-07-24T21:04:27Z

src/cdk/stepper/stepper.ts

@@ -126,6 +147,19 @@ export class CdkStep implements OnChanges {
    return this.stepControl ? this.stepControl.valid && this.interacted : this.interacted;
  }

+  /** Whether step has error. */
+  get hasError(): boolean {


should this be an @Input?

Done. I will also be creating a showError input as mentioned in my other comment.

mmalerba · 2018-07-24T21:06:16Z

src/cdk/stepper/stepper.ts

+  }
+  private _customError: boolean | null = null;
+
+  private get _defaultError() {


make this a method called _getDefaultError, we don't like to use getter/setters for private properties since it generates more code

I was following the pattern made for the completed attributes, where it says private get _defaultCompleted(). What's the difference between that and private get _defaultError()? Should they both be changed? Please advise!

The other one should probably not be a getter either

src/cdk/stepper/stepper.ts

mmalerba · 2018-07-24T21:09:39Z

src/cdk/stepper/stepper.ts

@@ -87,6 +98,9 @@ export class CdkStep implements OnChanges {
  /** Plain text label of the step. */
  @Input() label: string;

+  /** Alert message when there's an error. */
+  @Input() alertMessage: string;


call this errorMessage to keep the terminology consistent

googlebot · 2018-07-26T17:49:09Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-26T19:24:50Z

CLAs look good, thanks!

josephperrott · 2018-07-31T15:58:08Z

src/cdk/stepper/stepper.ts

@@ -112,6 +126,22 @@ export class CdkStep implements OnChanges {
  }
  private _optional = false;

+  /** State of the step. */
+  @Input()
+  get state(): StepState | string | null { return this._state; }


There should not be any need to have an internal private version of state if the getter and setter of state just set and get _state.

You should be able to just have

@Input() state: StepState | string | null = null;

Same for showError

Done! It is now a simple Input().

josephperrott · 2018-07-31T16:00:31Z

src/cdk/stepper/stepper.ts

@@ -112,6 +126,22 @@ export class CdkStep implements OnChanges {
  }
  private _optional = false;

+  /** State of the step. */
+  @Input()
+  get state(): StepState | string | null { return this._state; }


I am a bit confused on the typings for state, wouldn't it be best to capture all of the appropriate states in StepState instead of allowing arbitrary strings? Additionally, should null be possible, is one of the StepStates a reasonable default?

Yes, makes sense! So, I've added string as part of the StepState type. And correct, it should probably never be null; therefore, I've added STEP_STATE.NUMBER as the default. Thanks!

josephperrott · 2018-07-31T16:01:06Z

src/cdk/stepper/stepper.ts

+  /** Whether step has error. */
+  @Input()
+  get hasError(): boolean {
+    return this._customError == null ? this._getDefaultError : this._customError;


=== is preferred to ==

If you just want to do a truthyness check in this case, you could use:

return this._customError || this._getDefaultError;

Perfect, thanks!

josephperrott · 2018-07-31T16:03:17Z

src/cdk/stepper/stepper.ts

+  }
+  private _customError: boolean | null = null;
+
+  private get _getDefaultError() {


Getters end up creating alot of extra boilerplate code. For something like this, can you instead have a private method?

private _getDefaultError() { return this.stepControl && this.stepControl.invalid; }

I have refactored this method as well as the _getDefaultCompleted method. Thanks!

arodr967 · 2018-08-06T14:51:28Z

Mon. 08/06/18
@mmalerba Looks like one of the travis-ci jobs errored out due to external reasons. Would it be possible for you to rebuild it? I don't seem to have access to trigger the build again. Thanks!

Wed. 08/08/18
Seems like the build has restarted on it's own and it's now green.

arodr967 · 2018-08-07T17:27:12Z

@mmalerba Hey! Please let me know if the changes that I've made so far are good so that I can write some tests and hopefully get this PR merged. Thanks!

feat: added error state in order to display the correct icon and a default state to display any custom state/icon refactor: add StepState type refactor: refactor getIndicatorType to become more readable feat: add stepper example using a custom state feat: add error step theme feat: update getIndicatorType logic to handle a custom state feat: add alertMessage input for the step and conditionally apply error css feat: add icon override demo example and allow any icon to be overridden fix: issue with icon colors when selected or not test: update/add unit tests for the stepper changes test: update e2e tests for the stepper changes docs(stepper): update the readme to include new functionality refactor: add missing comment refactor: use mixins for stepper theme error

…dd completed and hasError as an Input, rename alertMessage to errorMessage

…IndicatorType

…e, add spacing in scss, and use .first function in tests

…p-icon-not-touched

mmalerba · 2018-08-28T17:41:29Z

src/lib/stepper/step-header.html

@@ -1,6 +1,10 @@
 <div class="mat-step-header-ripple" mat-ripple [matRippleTrigger]="_getHostElement()"></div>
-<div [class.mat-step-icon]="state !== 'number' || selected"
+<!-- @breaking-change 8.0.0 remove `mat-step-icon-not-touched` -->
+<div class="mat-step-icon"


@arodr967 Hmm ok, it looks like maybe the touched class wasn't the issue after all. I'm still seeing screenshot diffs where the pencil is changed from blue to gray. Looking a little deeper I think the issue may be related to the mat-step-icon class. It looks like you changed it from conditionally applied to always applied and also changed the styles targeted at it to no longer set the background color. I'm assuming that's where these diffs are coming from. How do you feel about changing the classes as follows:

<div class="mat-step-icon mat-step-icon-state-{{state}}" [class.mat-step-icon-selected]="selected" [ngSwitch]="state"> ...

To me this seems nicer anyways, because each state will get its own class that users can target. If making the pencil gray by default was intentional, I can just add CSS in google to target the -state-edit class and make it blue again. And we can probably just remove the -not-touched class again since it seems it wasn't the cause of the problem. I can use the -state- classes and the -selected class to target the same elements anyways if needed.

Sorry for all the back and forth on this, sorting out the issues in google3 turned out to be more difficult than I anticipated

No worries!
So I am not sure mat-step-icon-state-{{state}} would work for the custom defined states... And no, making the pencil gray by default was not intentional.
I'll get started on the changes.

Why would it not work for custom states? As long as they provide a string it should just insert it into the class name right?

Yes but I wouldn't be able to predefine the classes since there could be many different types. Or would that be something that the developer could do on their side? Define the class? 🤔
Answering my own question -- yes!

You don't need to target styles at all of them, you might want to for the predefined states like -error but we can still automatically add the class for other states so that users can write styles for .mat-step-state-mystate and it will just work

Simple way to fix the problem you're describing with the gray pencil is by just adding additional logic to mat-step-icon-selected and mat-step-icon-not-selected classes like so:

<div class="mat-step-icon" [class.mat-step-icon-selected]="selected || state == 'done' || state == 'edit'" [class.mat-step-icon-not-selected]="!selected && state != 'done' && state != 'edit'" [class.mat-step-icon-error]="state == 'error'" [ngSwitch]="state">

Let me know what you think!

I would rather change the classes to be like I suggested. It also makes the meaning of the -selected class more clear. I would expect the class to reflect the selected property, not all of these other conditions. I also think its useful for the user if we expose classes for their custom states

Okay, I'll see what I can do in that case, thanks!

Sorry took a while to get this done, been really busy! I have pushed the changes. Let me know what you think.
I had to make this !important:

.mat-step-icon-selected, .mat-step-icon-state-done, .mat-step-icon-state-edit { background-color: mat-color($primary) !important; color: mat-color($primary, default-contrast); }

Otherwise, the styles wouldn't get applied correctly!

mmalerba · 2018-09-06T16:51:07Z

src/lib/stepper/step-header.html

@@ -1,6 +1,5 @@
 <div class="mat-step-header-ripple" mat-ripple [matRippleTrigger]="_getHostElement()"></div>
-<div [class.mat-step-icon]="state !== 'number' || selected"
-     [class.mat-step-icon-not-touched]="state == 'number' && !selected"
+<div class="mat-step-icon-{{selected ? 'selected' : 'not-selected'}} mat-step-icon-state-{{state}} mat-step-icon"


can we just use .mat-icon:not(.mat-step-icon-selected) instead of having both classes?

sounds good!

mmalerba · 2018-09-06T18:10:15Z

src/lib/stepper/step-header.html

@@ -1,5 +1,6 @@
 <div class="mat-step-header-ripple" mat-ripple [matRippleTrigger]="_getHostElement()"></div>
-<div class="mat-step-icon-{{selected ? 'selected' : 'not-selected'}} mat-step-icon-state-{{state}} mat-step-icon"
+<div [ngClass]="{'mat-step-icon-selected': selected}"


[class.mat-step-icon-selected]="selected"

mmalerba · 2018-09-06T18:30:01Z

src/lib/stepper/_stepper-theme.scss

      background-color: mat-color($foreground, disabled-text);
      color: mat-color($primary, default-contrast);
    }

+    .mat-step-icon-state-error {
+      background-color: transparent !important;


We can't have !important anywhere in our CSS. I assume this is because you need greater specificity due to the .mat-step-icon:not(.mat-step-icon-selected) selector? Maybe instead we can just order them so that the right one takes precedence:

// default that will apply to every icon .mat-step-icon { background-color: mat-color($foreground, disabled-text); color: mat-color($primary, default-contrast); } // override background for icons that are selected or are in the edit/done state .mat-step-icon-selected, .mat-step-icon-state-done, .mat-step-icon-state-edit { background-color: mat-color($primary); } // override background and foreground for error state (will also take precedence over selected style) .mat-step-icon-state-error { background-color: transparent; color: mat-color($warn); }

I think the above is the correct order, but you should confirm. See https://www.w3schools.com/css/css_specificity.asp for more about CSS selector specificity

Ok I was able to remove the !important by using css specificity but it would not work with the not() selector so I had to use the -not-selected class again.

Ok, I pushed code that I think should be equivalent to what you had but doesn't rely on the class

…tant

arodr967 · 2018-09-06T21:29:46Z

@mmalerba Thanks for the help!

mmalerba · 2018-09-14T16:21:47Z

🎉

angular-automatic-lock-bot · 2019-09-09T14:03:46Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

arodr967 requested a review from mmalerba as a code owner May 22, 2018 18:26

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 22, 2018

mmalerba requested a review from jelbourn May 22, 2018 18:42

mmalerba self-assigned this Jul 19, 2018

mmalerba reviewed Jul 26, 2018

View reviewed changes

arodr967 requested review from amcdnl, andrewseguin, crisbeto, devversion and josephperrott as code owners July 26, 2018 17:47

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Jul 26, 2018

arodr967 force-pushed the stepper-states branch from 6fcb228 to 7973e5f Compare July 26, 2018 19:24

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jul 26, 2018

josephperrott reviewed Jul 31, 2018

View reviewed changes

arodr967 force-pushed the stepper-states branch from d4d0f18 to dd8036a Compare August 6, 2018 03:37

arodr967 changed the title ~~feat(stepper): add state input to step~~ feat(stepper): Create MAT_STEPPER_GLOBAL_OPTIONS InjectionToken Aug 6, 2018

arodr967 and others added 13 commits August 24, 2018 09:43

feat(stepper): add showError Input, update _getIndicatorType logic, a…

4adb591

…dd completed and hasError as an Input, rename alertMessage to errorMessage

feat(stepper): create stepper global options and update logic in _get…

beda45f

…IndicatorType

test(stepper): revert unit and e2e tests

1cc2f91

refactor(stepper-demo): revert to previous state

4d671f6

refactor(stepper): fix lint issues

820dc54

refactor(stepper): parameter formatting and fix typo

b3d5134

test(stepper): refactor stepper unit tests and add new tests

fdbf799

docs(stepper): add info about MAT_STEPPER_GLOBAL_OPTIONS

ebf59a4

refactor(stepper): remove extra string from some types using StepState

f1e3b02

refactor(stepper): rename useGuidelines to displayDefaultIndicatorTyp…

9f48437

…e, add spacing in scss, and use .first function in tests

docs(stepper): add embedded examples

726ce77

fix(stepper): make stepperOptions an optional parameter

ed4e121

arodr967 force-pushed the stepper-states branch from 29bb9ca to ed4e121 Compare August 24, 2018 13:58

arodr967 added 3 commits August 24, 2018 10:46

refactor(stepper): rename mat-step-icon-not-selected class to mat-ste…

4e952fe

…p-icon-not-touched

feat(stepper): only show step error after it has been interacted with

ea4ded3

fix(stepper): add back both not-touched and not-selected css classes

b3d11ef

mmalerba reviewed Aug 28, 2018

View reviewed changes

fix(stepper): add state specific css classes

e5bceb3

mmalerba reviewed Sep 6, 2018

View reviewed changes

refactor(stepper): use not selector instead of extra css class

f2ccadc

mmalerba reviewed Sep 6, 2018

View reviewed changes

arodr967 and others added 2 commits September 6, 2018 15:16

fix(stepper): use css specificity in order to remove the use of impor…

8cd1910

…tant

remove -not-selected class

10fc780

jelbourn merged commit 9ab2c90 into angular:master Sep 14, 2018

arodr967 deleted the stepper-states branch September 14, 2018 14:11

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019

feat(stepper): Create MAT_STEPPER_GLOBAL_OPTIONS InjectionToken #11457

feat(stepper): Create MAT_STEPPER_GLOBAL_OPTIONS InjectionToken #11457

Uh oh!

Conversation

arodr967 commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Major

Uh oh!

zackarychapple commented May 22, 2018

Uh oh!

mmalerba commented May 22, 2018

Uh oh!

zackarychapple commented May 22, 2018

Uh oh!

ngbot bot commented Jul 11, 2018

Uh oh!

ngbot bot commented Jul 11, 2018

Uh oh!

ngbot bot commented Jul 11, 2018

Uh oh!

zackarychapple commented Jul 19, 2018

Uh oh!

arodr967 commented Jul 26, 2018

Uh oh!

mmalerba 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jul 26, 2018

Uh oh!

googlebot commented Jul 26, 2018

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!

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

Uh oh!

Uh oh!

arodr967 commented Aug 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

arodr967 commented May 22, 2018 •

edited

Loading

arodr967 commented Aug 6, 2018 •

edited

Loading

arodr967 Aug 30, 2018 •

edited

Loading

arodr967 Aug 30, 2018 •

edited

Loading

arodr967 Aug 30, 2018 •

edited

Loading

arodr967 Sep 5, 2018 •

edited

Loading