feat(stepper): Merge stepper branch into master #6594

g1shin · 2017-08-22T22:12:51Z

No description provided.

mmalerba · 2017-08-22T22:48:21Z

src/lib/stepper/_stepper-theme.scss

+@import '../core/theming/theming';
+@import '../core/typography/_typography-utils.scss';
+
+@mixin mat-stepper-theme($theme) {


should have typography mixin too (even if it currently does nothing)

rafaelss95 · 2017-08-23T01:33:18Z

src/demo-app/stepper/stepper-demo.ts

+    });
+
+    this.emailFormGroup = this._formBuilder.group({
+      emailCtrl: ['', Validators.pattern(EMAIL_REGEX)]


Validators.email can be used here: emailCtrl: ['', Validators.email]

rafaelss95 · 2017-08-23T01:42:06Z

src/lib/stepper/step-header.html

+     [class.mat-step-icon-not-touched]="icon == 'number' && !selected">
+  <span *ngIf="icon == 'number'">{{index + 1}}</span>
+  <md-icon *ngIf="icon == 'edit'">create</md-icon>
+  <md-icon *ngIf="icon == 'done'">done</md-icon>


I think ngSwitch fits better than 3 *ngIf.

[ngSwitch]="icon" *ngSwitchCase="number" ... *ngSwitchCase="edit"... *ngSwitchCase="done"... end

g1shin · 2017-08-23T02:14:38Z

All the checks are now passing 👍 Ready for review

mmalerba

LGTM

crisbeto · 2017-08-23T06:43:40Z

src/cdk/stepper/stepper.ts

+
+  /** The top level abstract control of the step. */
+  @Input()
+  get stepControl() { return this._stepControl; }


Why is this using a getter and a setter? It doesn't seem like it has any special logic. Using a normal property will result in a bit less generated JS.

crisbeto · 2017-08-23T06:46:34Z

src/cdk/stepper/stepper.ts

+})
+export class CdkStepper {
+  /** The list of step components that the stepper is holding. */
+  @ContentChildren(CdkStep) _steps: QueryList<CdkStep>;


Is there a reason for this, _stepHeader and _focusIndex to be public? It seems like they're only used internally.

Anything that's a @ContentChild(ren) or @ViewChild(ren) as well as anything referenced in the template must be public, they start with _ to indicate they're internal

crisbeto · 2017-08-23T06:47:09Z

src/cdk/stepper/stepper.ts

+      return 'previous';
+    } else if (position > 0) {
+      return 'next';
+    } else {


The else here is redundant, you can just add the return 'current' at the end.

crisbeto · 2017-08-23T06:48:56Z

src/cdk/stepper/stepper.ts

+  }
+
+  private _emitStepperSelectionEvent(newIndex: number): void {
+    const stepsArray = this._steps.toArray();


It seems like there are a lot of places where you're converting the steps into an array in order to look something up. It might be cleaner if you stored the selected step object instead of its index.

crisbeto · 2017-08-23T06:49:53Z

src/cdk/stepper/stepper.ts

+
+  _onKeydown(event: KeyboardEvent) {
+    switch (event.keyCode) {
+      case RIGHT_ARROW:


This doesn't account for RTL where the directions are inverted.

crisbeto · 2017-08-23T07:03:14Z

src/lib/stepper/stepper-vertical.ts

+  animations: [
+      trigger('stepTransition', [
+        state('previous', style({height: '0px', visibility: 'hidden'})),
+        state('next', style({height: '0px', visibility: 'hidden'})),


Since the previous and next states have the same styling, you can combine them:

state('next, previous', style({height: '0px', visibility: 'hidden'}))

crisbeto · 2017-08-23T07:04:27Z

src/lib/stepper/stepper.md

+```html
+<md-vertical-stepper>
+  <md-step label="Step 1">
+    Content 1


Consider using some real-world text for the example, instead of Content X.

crisbeto · 2017-08-23T07:07:06Z

src/lib/stepper/stepper.spec.ts

+import {ENTER, LEFT_ARROW, RIGHT_ARROW, SPACE} from '@angular/cdk/keycodes';
+import {MdStepper} from './stepper';
+
+const EMAIL_REGEX = /^[a-zA-Z0-9.!#$%&’*+/=?^_`{|}~-]+@[a-zA-Z0-9-]+(?:\.[a-zA-Z0-9-]+)*$/;


Since the unit test doesn't really care about what you're validating, you can use a simpler regex that only validates a certain value, e.g. const UNICORN_REGEX = /unicorn/.

crisbeto · 2017-08-23T07:10:00Z

src/lib/stepper/stepper.spec.ts

+
+    it('should support keyboard events to move and select focus', () => {
+      let stepHeaders = fixture.debugElement.queryAll(By.css('.mat-horizontal-stepper-header'));
+      assertCorrectKeyboardInteraction(stepperComponent, fixture, stepHeaders);


Not a fan of having the assertions be in separate functions, because it makes failures harder to track down, although in this case the assertions seem pretty complicated. Thoughts @jelbourn?

crisbeto · 2017-08-23T07:13:16Z

src/lib/stepper/stepper.ts

+  constructor(mdStepper: MdStepper,
+              @Optional() @SkipSelf() @Inject(MD_ERROR_GLOBAL_OPTIONS) errorOptions: ErrorOptions) {
+    super(mdStepper);
+    this._originalErrorStateMatcher =


The ternary with a nested || is a little hard to follow. It might be cleaner if it used a regular if/else.

mmalerba · 2017-08-23T15:03:38Z

@g1shin please address @rafaelss95's and @crisbeto's stylistic comments in a follow-up PR. The RTL issue that @crisbeto pointed out is a bug and should be fixed in this PR before merging

mmalerba · 2017-08-23T16:10:44Z

fixed RTL bugs

@input

…tepper branch. (#5742) * Prototyping * Further work * Further prototyping * Further prototyping * Further work * Adding event emitters * Adding "selectedIndex" attribute to stepper and working on TemplateOulet. * Prototyping * Further work * Further prototyping * Further prototyping * Further work * Adding event emitters * Template rendering and selectIndex control done. * Work in progress for accessibility * Added functionalities based on the tentative API doc. * Refactor code for cdk-stepper and cdk-step * Add support for templated label * Added support for keyboard events and focus changes for accessibility. * Updated vertical stepper + added comments * Fix package-lock.json * Fix indention * Changes made based on the review * Changes based on review - event properties, selectors, SPACE support, etc. + demo * Add select() for step component + refactor to avoid circular dependency + support cycling using arrow keys * API change based on review * Minor code clean up based on review. * Several name changes, etc based on review * Add to compatibility mode list and refactor to avoid circular dependency feat(stepper): Create stepper button directives to enable adding buttons to stepper (#5951) * Create stepper button directives to enable adding buttons to stepper * Changes made based on review * Minor changes with click handlers Build changes feat(stepper): Add initial styles to stepper based on Material guidelines (#6242) * Add initial styles to stepper based on Material guidelines * Fix flex-shrink and min-width * Changes made based on review * Fix alignment * Margin modifications feat(stepper): Add support for linear stepper (#6116) * Add form controls and custom error state matcher * Modify form controls for stepper-demo and add custom validator * Move custom step validation function so that users can simply import and use * Implement @input() stepControl for each step * Add linear attribute to stepper * Add enabling/disabling linear state of demo feat(stepper): Add animation to stepper (#6361) * Add animation * Implement Angular animation * Clean up unnecessary code * Generalize animation so that vertical and horizontal steppers can use the same function Rebase onto upstream/master feat(stepper): Add unit tests for stepper (#6428) * Add unit tests for stepper * Changes made based on review * More changes based on review feat(stepper): Add support for linear stepper #2 - each step as its own form. (#6117) * Add form control - consider each step as its own form group * Comment edits * Add 'valid' to MdStep for form validation * Add [stepControl] to each step based on merging * Changes based on review Fix focus logic and CSS changes (#6507) feat(stepper): Add documentation for stepper (#6533) * Documentation for stepper * Revision based on review + add accessibility section feat(stepper): Support additional properties for step (#6509) * Additional properties for step * Unit tests * Code changes based on review + test name changes * Refactor code for shared functionality between vertical and horizontal stepper * Refactor md-step-header and md-step-content + optional step change * Simplify code based on review * Changes to step-header based on review * Minor changes Fix host style and demo page (#6592) Revert package.json and package-lock.json Changes made along with BUILD changes in google3 Add typography mixin Changes to address aot compiler failures fix rtl bugs

angular-automatic-lock-bot · 2019-09-07T22:01:20Z

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.}

g1shin assigned jelbourn, kara, g1shin and mmalerba Aug 22, 2017

g1shin requested review from jelbourn, kara and mmalerba August 22, 2017 22:12

g1shin requested a review from devversion as a code owner August 22, 2017 22:12

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

g1shin removed the request for review from devversion August 22, 2017 22:13

mmalerba reviewed Aug 22, 2017

View reviewed changes

g1shin requested a review from crisbeto as a code owner August 23, 2017 00:32

g1shin force-pushed the stepper branch 2 times, most recently from 7ec40bf to 1bbb27f Compare August 23, 2017 01:27

rafaelss95 reviewed Aug 23, 2017

View reviewed changes

mmalerba approved these changes Aug 23, 2017

View reviewed changes

mmalerba added the pr: lgtm label Aug 23, 2017

crisbeto reviewed Aug 23, 2017

View reviewed changes

mmalerba added the action: merge The PR is ready for merge by the caretaker label Aug 23, 2017

mmalerba force-pushed the stepper branch from 67d654c to 7031e93 Compare August 23, 2017 16:19

mmalerba force-pushed the stepper branch from 7031e93 to 1e51fae Compare August 23, 2017 17:39

Remove getter and setter for stepControl input

f1df032

g1shin force-pushed the stepper branch from 85f0258 to f1df032 Compare August 23, 2017 20:40

jwshinjwshin added 2 commits August 28, 2017 10:12

Injection fix

1319d98

Code refactoring for google3

8fc6fe6

kara unassigned jelbourn, kara, g1shin and mmalerba Aug 31, 2017

jelbourn merged commit 87318bc into master Aug 31, 2017

g1shin mentioned this pull request Sep 8, 2017

md-stepper component #508

Closed

jelbourn deleted the stepper branch December 7, 2017 23:47

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stepper): Merge stepper branch into master #6594

feat(stepper): Merge stepper branch into master #6594

g1shin commented Aug 22, 2017

mmalerba Aug 22, 2017

rafaelss95 Aug 23, 2017 •

edited

Loading

rafaelss95 Aug 23, 2017

g1shin commented Aug 23, 2017

mmalerba left a comment

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

mmalerba Aug 23, 2017

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

crisbeto Aug 23, 2017

mmalerba commented Aug 23, 2017

mmalerba commented Aug 23, 2017

angular-automatic-lock-bot bot commented Sep 7, 2019

feat(stepper): Merge stepper branch into master #6594

feat(stepper): Merge stepper branch into master #6594

Conversation

g1shin commented Aug 22, 2017

Choose a reason for hiding this comment

rafaelss95 Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g1shin commented Aug 23, 2017

mmalerba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Aug 23, 2017

mmalerba commented Aug 23, 2017

angular-automatic-lock-bot bot commented Sep 7, 2019

rafaelss95 Aug 23, 2017 •

edited

Loading