Skip to content

Add tests for sticky-header #6074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 28 commits into from
Aug 4, 2017
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/lib/sticky-header/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@
import {NgModule} from '@angular/core';
import {CommonModule} from '@angular/common';
import {OverlayModule, MdCommonModule, PlatformModule} from '../core';
import {CdkStickyRegion, CdkStickyHeader} from './sticky-header';

import {CdkStickyRegion, CdkStickyHeader,
STICKY_HEADER_SUPPORT_STRATEGY_PROVIDER} from './sticky-header';


@NgModule({
imports: [OverlayModule, MdCommonModule, CommonModule, PlatformModule],
declarations: [CdkStickyRegion, CdkStickyHeader],
exports: [CdkStickyRegion, CdkStickyHeader, MdCommonModule],
providers: [STICKY_HEADER_SUPPORT_STRATEGY_PROVIDER]
})
export class StickyHeaderModule {}

Expand Down
175 changes: 175 additions & 0 deletions src/lib/sticky-header/sticky-header.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
import {async, ComponentFixture, TestBed, fakeAsync, tick} from '@angular/core/testing';
import {Component, DebugElement, ViewChild} from '@angular/core';
import {StickyHeaderModule, CdkStickyRegion, CdkStickyHeader,
STICKY_HEADER_SUPPORT_STRATEGY} from './index';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you run the "optimize imports" command in webstorm it will format these imports for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

import {OverlayModule, Scrollable} from '../core/overlay/index';
import {PlatformModule} from '../core/platform/index';
import {By} from '@angular/platform-browser';
import {dispatchFakeEvent} from '@angular/cdk/testing';



describe('sticky-header with positioning not supported', () => {
let fixture: ComponentFixture<StickyHeaderTest>;
let testComponent: StickyHeaderTest;
let stickyElement: DebugElement;
let stickyParentElement: DebugElement;
let scrollableElement: DebugElement;
let stickyHeaderDir: CdkStickyHeader;

beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [ OverlayModule, PlatformModule, StickyHeaderModule ],
declarations: [StickyHeaderTest],
providers: [
{provide: STICKY_HEADER_SUPPORT_STRATEGY, useValue: false},
],
});
TestBed.compileComponents();
}));

beforeEach(() => {
fixture = TestBed.createComponent(StickyHeaderTest);
fixture.detectChanges();
testComponent = fixture.debugElement.componentInstance;
stickyElement = fixture.debugElement.query(By.directive(CdkStickyHeader));
stickyParentElement = fixture.debugElement.query(By.directive(CdkStickyRegion));
stickyHeaderDir = stickyElement.injector.get<CdkStickyHeader>(CdkStickyHeader);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just stickyHeader is a good name. Dir makes me think it is short for "direction"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Renamed stickyHeaderDir to stickyHeader

scrollableElement = fixture.debugElement.query(By.directive(Scrollable));
});

it('should be able to find stickyParent', () => {
expect(stickyHeaderDir.stickyParent).not.toBe(null);
});

it('should be able to find scrollableContainer', () => {
expect(stickyHeaderDir.upperScrollableContainer).not.toBe(null);
});

it('should stick in the right place when scrolled to the top of the container', fakeAsync(() => {
let scrollableContainerTop = stickyHeaderDir.upperScrollableContainer
.getBoundingClientRect().top;
expect(stickyHeaderDir.element.getBoundingClientRect().top).not.toBe(scrollableContainerTop);
tick(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the tick() functions needed and with specific values like 0 and 100? Can you leave comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Added comments to explaining why tick(100) is needed. Changed 'tick(0)' to 'tick()'

// Use tick(100) to tick forward to let stickyHeader's _applyStickyPositionStyles()
 // function finished


// Scroll the scrollableContainer up to stick
fixture.componentInstance.scrollDown();
tick(100);

expect(stickyHeaderDir.element.getBoundingClientRect().top).toBe(scrollableContainerTop);
}));

it('should unstuck when scrolled off the top of the container', fakeAsync(() => {
let scrollableContainerTop = stickyHeaderDir.upperScrollableContainer
.getBoundingClientRect().top;
expect(stickyHeaderDir.element.getBoundingClientRect().top).not.toBe(scrollableContainerTop);
tick(0);

// Scroll the scrollableContainer up to stick
fixture.componentInstance.scrollDown();
tick(100);

expect(stickyHeaderDir.element.getBoundingClientRect().top).toBe(scrollableContainerTop);

// Scroll the scrollableContainer down to unstuck
fixture.componentInstance.scrollBack();
tick(100);

expect(stickyHeaderDir.element.getBoundingClientRect().top).not.toBe(scrollableContainerTop);

}));
});

describe('sticky-header with positioning supported', () => {
let fixture: ComponentFixture<StickyHeaderTest>;
let testComponent: StickyHeaderTest;
let stickyElement: DebugElement;
let stickyParentElement: DebugElement;
let stickyHeaderDir: CdkStickyHeader;

beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [ OverlayModule, PlatformModule, StickyHeaderModule ],
declarations: [StickyHeaderTest],
providers: [
{provide: STICKY_HEADER_SUPPORT_STRATEGY, useValue: true},
],
});
TestBed.compileComponents();
}));

beforeEach(() => {
fixture = TestBed.createComponent(StickyHeaderTest);
fixture.detectChanges();
testComponent = fixture.debugElement.componentInstance;
stickyElement = fixture.debugElement.query(By.directive(CdkStickyHeader));
stickyParentElement = fixture.debugElement.query(By.directive(CdkStickyRegion));
stickyHeaderDir = stickyElement.injector.get<CdkStickyHeader>(CdkStickyHeader);
});

it('should find sticky positioning is applied', () => {
let position = window.getComputedStyle(stickyHeaderDir.element).position;
expect(position).not.toBe(null);
if (position != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The expectation already says its not null, do we need the condition != null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in expect(/sticky/i.test(position)).toBe(true);, the test() function's parameter can only be string. And the type of position is string | null, which will cause an error. S we need to add the '!= null' first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can tell typescript that you know something won't be null by adding an ! to the end of an expression:

expect(/sticky/i.test(position!)).toBe(true);

Then you don't need the if block

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Changed to expect(/sticky/i.test(position!)).toBe(true);

expect(/sticky/i.test(position)).toBe(true);
}
});
});

@Component({
template: `
<div cdk-scrollable style="text-align: center;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment that explains what these inline styles are for? Any reason you can't set them via the styles configuration for the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Added comments on why we need to apply these styles. And put set styles via the styles configuration for the component.

-webkit-appearance: none;
-moz-appearance: none;
height: 300px;
overflow: auto;">
<p>test test test</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than hard-coding a bunch of divs, you could use ngFor to expand the content

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Changed to use 'ngFor' to expand content instead of typing tons of '<p></p>'

<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<div cdkStickyRegion id="theStickyHeaderLalala">
<div cdkStickyHeader style="background: whitesmoke; padding: 5px;">
<h2>Heading 1</h2>
</div>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
<p>test test test</p>
</div>
</div>
`})
class StickyHeaderTest {
@ViewChild(Scrollable) scrollingContainer: Scrollable;

scrollDown() {
const scrollingContainerEl = this.scrollingContainer.getElementRef().nativeElement;
scrollingContainerEl.scrollTop = 300;

// Emit a scroll event from the scrolling element in our component.
dispatchFakeEvent(scrollingContainerEl, 'scroll');
}

scrollBack() {
const scrollingContainerEl = this.scrollingContainer.getElementRef().nativeElement;
scrollingContainerEl.scrollTop = 0;

// Emit a scroll event from the scrolling element in our component.
dispatchFakeEvent(scrollingContainerEl, 'scroll');
}
}
26 changes: 16 additions & 10 deletions src/lib/sticky-header/sticky-header.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
* found in the LICENSE file at https://angular.io/license
*/
import {Directive, Input,
OnDestroy, AfterViewInit, ElementRef, Optional} from '@angular/core';
import {Platform} from '../core/platform';
OnDestroy, AfterViewInit, ElementRef, Optional,
InjectionToken, Injectable, Inject, Provider} from '@angular/core';
import {Platform} from '../core/platform/index';
import {Scrollable} from '../core/overlay/scroll/scrollable';
import {extendObject} from '../core/util/object-extend';
import {Subscription} from 'rxjs/Subscription';
Expand All @@ -32,7 +33,6 @@ export class CdkStickyRegion {
constructor(public readonly _elementRef: ElementRef) { }
}


/** Class applied when the header is "stuck" */
const STICK_START_CLASS = 'cdk-sticky-header-start';

Expand All @@ -46,6 +46,14 @@ const STICK_END_CLASS = 'cdk-sticky-header-end';
*/
const DEBOUNCE_TIME: number = 5;

export const STICKY_HEADER_SUPPORT_STRATEGY = new InjectionToken('sticky-header-support-strategy');

/** Create a factory for sticky-positioning check to make code more testable */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably mark this as @docs-private

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Added /** @docs-private */

export const STICKY_HEADER_SUPPORT_STRATEGY_PROVIDER: Provider = {
provide: STICKY_HEADER_SUPPORT_STRATEGY,
useFactory: isPositionStickySupported
};

/**
* Directive that marks an element as a sticky-header. Inside of a scrolling container (marked with
* cdkScrollable), this header will "stick" to the top of the scrolling viewport while its sticky
Expand All @@ -61,8 +69,6 @@ export class CdkStickyHeader implements OnDestroy, AfterViewInit {

/** boolean value to mark whether the current header is stuck*/
isStuck: boolean = false;
/** Whether the browser support CSS sticky positioning. */
private _isPositionStickySupported: boolean = false;

/** The element with the 'cdkStickyHeader' tag. */
element: HTMLElement;
Expand Down Expand Up @@ -97,7 +103,8 @@ export class CdkStickyHeader implements OnDestroy, AfterViewInit {
constructor(element: ElementRef,
scrollable: Scrollable,
@Optional() public parentRegion: CdkStickyRegion,
platform: Platform) {
platform: Platform,
@Inject(STICKY_HEADER_SUPPORT_STRATEGY) public _isPositionStickySupported) {
if (platform.isBrowser) {
this.element = element.nativeElement;
this.upperScrollableContainer = scrollable.getElementRef().nativeElement;
Expand Down Expand Up @@ -137,7 +144,6 @@ export class CdkStickyHeader implements OnDestroy, AfterViewInit {
* sticky positioning. If not, use the original implementation.
*/
private _setStrategyAccordingToCompatibility(): void {
this._isPositionStickySupported = isPositionStickySupported();
if (this._isPositionStickySupported) {
this.element.style.top = '0';
this.element.style.cssText += 'position: -webkit-sticky; position: sticky; ';
Expand Down Expand Up @@ -258,9 +264,9 @@ export class CdkStickyHeader implements OnDestroy, AfterViewInit {


/**
* '_applyStickyPositionStyles()' function contains the main logic of sticky-header. It decides when
* a header should be stick and when should it be unstuck by comparing the offsetTop
* of scrollable container with the top and bottom of the sticky region.
* '_applyStickyPositionStyles()' function contains the main logic of sticky-header.
* It decides when a header should be stick and when should it be unstuck by comparing
* the offsetTop of scrollable container with the top and bottom of the sticky region.
*/
_applyStickyPositionStyles(): void {
let currentPosition: number = this.upperScrollableContainer.offsetTop;
Expand Down