feat(stepper): Address previous comments + add directionality support #6775
Conversation
googlebot
added
the
cla: yes
|
added @crisbeto since many of the comments were his |
src/cdk/stepper/stepper.ts
Outdated
| }); | ||
| this._selectedIndex = newIndex; | ||
| } | ||
|
|
||
| _onKeydown(event: KeyboardEvent) { | ||
| switch (event.keyCode) { | ||
| case RIGHT_ARROW: | ||
| if (this._dir && this._dir.value === 'rtl') { | ||
| if (this._layoutDirection() === 'rtl') { | ||
| this._focusStep((this._focusIndex + this._steps.length - 1) % this._steps.length); |
There was a problem hiding this comment.
since these previous and next expressions are repeated and kind of intimidating it might be nice for readability to create _focusNextStep and _focusPreviousStep methods
src/cdk/stepper/stepper.ts
Outdated
| @@ -167,6 +169,10 @@ export class CdkStepper { | |||
| this._groupId = nextId++; | |||
| } | |||
|
|
|||
| ngAfterContentInit() { | |||
| this._stepsArray = this._steps.toArray(); | |||
There was a problem hiding this comment.
Can't do this since _steps may update, e.g. when an ngIf changes from true to false
src/lib/stepper/_stepper-theme.scss
Outdated
| @@ -13,11 +13,12 @@ | |||
| background-color: mat-color($background, hover); | |||
| } | |||
|
|
|||
| .mat-step-label-active { | |||
| .mat-step-label.mat-step-label-active { | |||
There was a problem hiding this comment.
nit: I like to put the more specific ones after
src/lib/stepper/stepper.spec.ts
Outdated
| @@ -364,30 +421,47 @@ function assertPreviousStepperButtonClick(stepperComponent: MdStepper, | |||
| } | |||
|
|
|||
| /** Asserts that step position is correct for animation. */ | |||
| function assertCorrectStepPosition(stepperComponent: MdStepper, fixture: ComponentFixture<any>) { | |||
| function assertCorrectStepPosition(stepperComponent: MdStepper, | |||
There was a problem hiding this comment.
Can we rename this to assertCorrectStepAnimationDirection? I'm not really sure what "position" refers to. Also we can determine the stepper automatically given the fixture and take in a optional direction ('ltr' or 'rtl') rather than a boolean so that we aren't always passing dir === 'rtl'
There was a problem hiding this comment.
What about automatically getting the stepper from the fixture (in this method and others)?
There was a problem hiding this comment.
@mmalerba stepper is obtained from fixture by css query (e.g fixture.debugElement.query(By.css('md-horizontal-stepper')).componentInstance). Since these test functions are used commonly for both horizontal and vertical stepper, the stepper would need to be obtained first based on different test cases and then passed to the function for testing.
It could be possible to pass in a variable that checks to see if it's supposed to be horizontal or vertical stepper component then get the stepper from the fixture accordingly, but I'm not sure if that would be cleaner than how it's currently done.
src/cdk/stepper/stepper.ts
Outdated
| @@ -150,7 +152,7 @@ export class CdkStepper { | |||
| @Input() | |||
| get selected() { return this._steps[this.selectedIndex]; } | |||
| set selected(step: CdkStep) { | |||
| let index = this._steps.toArray().indexOf(step); | |||
| let index = this._stepsArray.indexOf(step); | |||
There was a problem hiding this comment.
You can inline this assignment: this.selectedIndex = this._stepsArray.indexOf(step).
src/cdk/stepper/stepper.ts
Outdated
| if (this._linear) { | ||
| return stepsArray.slice(0, index).some(step => step.stepControl.invalid); | ||
| return this._stepsArray.slice(0, index).some(step => step.stepControl.invalid); |
There was a problem hiding this comment.
The index here may end up being a negative number (it corresponds to the selectedIndex which is a part of the public API) which will result in the some skipping the last N items of the array (e.g. [1, 2, 3, 4].slice(0, -1) is [1, 2, 3]).
|
Comments have been addressed. Ready for review again 👍 |
|
Modified test functions in stepper.spec.ts. Ready for review now |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Comments that were not addressed in the merging of first stepper PR into master branch are addressed in this PR.