feat(stepper): Add unit tests for stepper #6428

g1shin · 2017-08-12T01:29:52Z

No description provided.

shlomiassaf · 2017-08-14T09:45:14Z

@g1shin @jelbourn Been playing with the stepper branch.

Any particular reason for the label's index not being part of the whole custom label?

Instead of:

<div class="mat-stepper-index">{{i + 1}}</div>
<div class="mat-stepper-label">
  <!-- If there is a label template, use it. -->
  <ng-container *ngIf="step.stepLabel" [ngTemplateOutlet]="step.stepLabel.template"></ng-container>
  <!-- It there is no label template, fall back to the text label. -->
  <div *ngIf="!step.stepLabel">{{step.label}}</div>
</div>

do it like:

<div class="mat-stepper-label">
  <ng-container *ngIf="step.stepLabel" [ngTemplateOutlet]="step.stepLabel.template"></ng-container>
  <!-- It there is no label template, fall back to the text label. -->
  <div *ngIf="!step.stepLabel">
    <div class="mat-stepper-index">
      {{i + 1}}
    </div>
    {{step.label}}
  </div>
</div>

This way if a user opts-in for a custom label he will decide if to include the index or not.

Forcing the index does not allow real customization of the index, for example if I want to do roman numerals or A. B. C

Since the user will do *ngFor anyway, he will have the index...

shlomiassaf · 2017-08-14T09:49:16Z

Another thing that pops out is not using ViewEncapsulation.None in the component.

This does not align with other components...

EDIT: Also not using changeDetection: ChangeDetectionStrategy.OnPush

I comment here since opening an issue on something that does not exist on master branch might be confusing.

mmalerba · 2017-08-14T19:58:38Z

@g1shin needs rebase

g1shin · 2017-08-14T20:05:24Z

Rebase done; ready for review 👍

kara · 2017-08-14T23:15:50Z

src/lib/stepper/stepper.spec.ts

+import {dispatchKeyboardEvent} from '@angular/cdk/testing';
+import {ENTER, LEFT_ARROW, RIGHT_ARROW, SPACE} from '@angular/cdk/keycodes';
+
+fdescribe('MdHorizontalStepper', () => {


Looks like you left an fdescribe by mistake. Remove?

kara · 2017-08-14T23:16:23Z

src/lib/stepper/stepper.spec.ts

+fdescribe('MdHorizontalStepper', () => {
+  beforeEach(async(() => {
+    TestBed.configureTestingModule({
+      imports: [MdStepperModule, NoopAnimationsModule, FormsModule, ReactiveFormsModule],


Do you need FormsModule? It looks like you aren't using any template-driven directives. Same on line 114.

kara · 2017-08-14T23:17:28Z

src/lib/stepper/stepper.spec.ts

+    });
+
+    it('should change selected index on header click', () => {
+      let stepHeader = fixture.debugElement.queryAll(By.css('.mat-horizontal-stepper-header'));


Nit: Maybe name this stepHeaders (plural)? To make clear that this grabs headers from all steps. Same on line 142.

kara · 2017-08-14T23:19:58Z

src/lib/stepper/stepper.spec.ts

+  fixture.detectChanges();
+
+  expect(stepContentEl.getAttribute('aria-expanded')).toBe('false');
+  stepContentEl = stepContent[1].nativeElement;


Nit: It's a bit hard to follow when you're naming two different step elements with the same name. Maybe create a new let for this - secondStepEl?

kara · 2017-08-14T23:20:39Z

src/lib/stepper/stepper.spec.ts

+      expect(stepperEl.getAttribute('role')).toBe('tablist');
+    });
+
+    it('should expand the right content', () => {


Nit: In this case, you mean set aria-expanded correctly, right? I'd mention that in the test name. Same on line 152.

kara · 2017-08-14T23:23:48Z

src/lib/stepper/stepper.spec.ts

+
+    it('should not move to next step if current step is not valid', () => {
+      expect(testComponent.oneGroup.get('oneCtrl')!.value).toBe('');
+      expect(testComponent.oneGroup.valid).toBe(false);


Nit: Here I'd expect a test that the control itself is invalid (if you were to add another control, it could cause the parent form to be false for a different reason). Same on 199.

kara · 2017-08-14T23:25:03Z

src/lib/stepper/stepper.spec.ts

+  });
+});
+
+fdescribe('MdVerticalStepper', () => {


kara · 2017-08-14T23:28:34Z

src/lib/stepper/stepper.spec.ts

+  dispatchKeyboardEvent(stepHeaderEl, 'keydown', RIGHT_ARROW);
+  fixture.detectChanges();
+
+  expect(stepperComponent._focusIndex).toBe(1);


I think fallback messages for these would be helpful to guide us through your thinking.

kara · 2017-08-14T23:29:15Z

src/lib/stepper/stepper.spec.ts

+      <md-step [stepControl]="oneGroup">
+        <form [formGroup]="oneGroup">
+          <ng-template mdStepLabel>Step one</ng-template>
+          <md-input-container>


Do you want to use md-form-field on these to be forward-friendly? cc @mmalerba

+1 if you're synced up to where md-form-field exists

g1shin · 2017-08-15T17:15:11Z

Changes have been made based on review. Ready for review again 😃

kara

LGTM

kara · 2017-08-15T17:22:39Z

@mmalerba Do you want to take a look before I apply merge labels?

mmalerba · 2017-08-15T18:55:06Z

src/lib/stepper/stepper.spec.ts

+  describe('basic horizontal stepper', () => {
+    let fixture: ComponentFixture<SimpleMdHorizontalStepperApp>;
+    let stepperComponent: MdHorizontalStepper;
+    let stepperEl: HTMLElement;


since only 1 test uses this you can just declare it in that test

mmalerba · 2017-08-15T18:57:16Z

src/lib/stepper/stepper.spec.ts

+});
+
+function checkSelectionChangeOnHeaderClick(stepperComponent:
+                                               MdHorizontalStepper | MdVerticalStepper,


just use common base class MdStepper? (here and other functions that take a stepper)

mmalerba · 2017-08-15T18:58:42Z

src/lib/stepper/stepper.spec.ts

+      <md-step [stepControl]="oneGroup">
+        <form [formGroup]="oneGroup">
+          <ng-template mdStepLabel>Step one</ng-template>
+          <md-input-container>


+1 if you're synced up to where md-form-field exists

g1shin · 2017-08-15T19:46:27Z

Ready for review again

mmalerba · 2017-08-15T19:51:03Z

src/lib/stepper/stepper.spec.ts

+    it('should change selected index on header click', () => {
+      let stepHeaders = fixture.debugElement.queryAll(By.css('.mat-horizontal-stepper-header'));
+      checkSelectionChangeOnHeaderClick(stepperComponent, fixture, stepHeaders);
+


nit: remove extra line

* Add unit tests for stepper * Changes made based on review * More changes based on review

@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

@input

…tepper branch. (angular#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 (angular#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 (angular#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 (angular#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 (angular#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 (angular#6428) * Add unit tests for stepper * Changes made based on review * More changes based on review feat(stepper): Add support for linear stepper angular#2 - each step as its own form. (angular#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 (angular#6507) feat(stepper): Add documentation for stepper (angular#6533) * Documentation for stepper * Revision based on review + add accessibility section feat(stepper): Support additional properties for step (angular#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 (angular#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

@input

…tepper branch. (angular#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 (angular#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 (angular#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 (angular#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 (angular#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 (angular#6428) * Add unit tests for stepper * Changes made based on review * More changes based on review feat(stepper): Add support for linear stepper angular#2 - each step as its own form. (angular#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 (angular#6507) feat(stepper): Add documentation for stepper (angular#6533) * Documentation for stepper * Revision based on review + add accessibility section feat(stepper): Support additional properties for step (angular#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 (angular#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-06T21:02:28Z

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 12, 2017

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

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

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

g1shin removed the request for review from devversion August 12, 2017 01:30

g1shin force-pushed the stepper branch from 7cdbc40 to 2bdc6f3 Compare August 14, 2017 18:21

g1shin force-pushed the tests branch from 596ce36 to bb17154 Compare August 14, 2017 20:04

Add unit tests for stepper

059a010

g1shin force-pushed the tests branch from bb17154 to 059a010 Compare August 14, 2017 21:06

kara reviewed Aug 14, 2017

View reviewed changes

kara removed their assignment Aug 14, 2017

kara added the pr: needs review label Aug 14, 2017

Changes made based on review

60e8044

kara approved these changes Aug 15, 2017

View reviewed changes

kara added pr: lgtm and removed pr: needs review labels Aug 15, 2017

mmalerba reviewed Aug 15, 2017

View reviewed changes

mmalerba approved these changes Aug 15, 2017

View reviewed changes

More changes based on review

9933bbc

g1shin force-pushed the tests branch from d5ead6b to 9933bbc Compare August 15, 2017 19:54

g1shin merged commit 39a3649 into angular:stepper Aug 15, 2017

g1shin pushed a commit that referenced this pull request Aug 16, 2017

feat(stepper): Add unit tests for stepper (#6428)

43f9162

* Add unit tests for stepper * Changes made based on review * More changes based on review

g1shin pushed a commit to g1shin/material2 that referenced this pull request Aug 22, 2017

feat(stepper): Add unit tests for stepper (angular#6428)

7f50789

* Add unit tests for stepper * Changes made based on review * More changes based on review

g1shin pushed a commit to g1shin/material2 that referenced this pull request Aug 22, 2017

feat(stepper): Add unit tests for stepper (angular#6428)

a901a2e

* Add unit tests for stepper * Changes made based on review * More changes based on review

g1shin pushed a commit to g1shin/material2 that referenced this pull request Aug 22, 2017

feat(stepper): Add unit tests for stepper (angular#6428)

5a4e555

* Add unit tests for stepper * Changes made based on review * More changes based on review

g1shin pushed a commit that referenced this pull request Aug 23, 2017

feat(stepper): Add unit tests for stepper (#6428)

6d616d8

* Add unit tests for stepper * Changes made based on review * More changes based on review

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

feat(stepper): Add unit tests for stepper #6428

feat(stepper): Add unit tests for stepper #6428

Uh oh!

Conversation

g1shin commented Aug 12, 2017

Uh oh!

shlomiassaf commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shlomiassaf commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmalerba commented Aug 14, 2017

Uh oh!

g1shin commented Aug 14, 2017

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!

g1shin commented Aug 15, 2017

Uh oh!

kara left a comment

Choose a reason for hiding this comment

Uh oh!

kara commented Aug 15, 2017

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!

g1shin commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

shlomiassaf commented Aug 14, 2017 •

edited

Loading

shlomiassaf commented Aug 14, 2017 •

edited

Loading