feat(sticky-header): Initial version of sticky header new PRNew Sticky header #5858
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
add chose parent add support to 'optional 'cdkStickyRegion' input ' add app-demo for sticky-header fix bugs and deleted unused tag id in HTML files modify
…)' function, add its content into 'ngOnDestroy()'.
Use // for comments, /* */ for docs
sllethe
commented
Jul 21, 2017
src/lib/module.ts
Outdated
| @@ -86,7 +87,9 @@ const MATERIAL_MODULES = [ | |||
| A11yModule, | |||
| PlatformModule, | |||
| MdCommonModule, | |||
| ObserveContentModule | |||
| ObserveContentModule, | |||
| CdkDataTableModule, | |||
There was a problem hiding this comment.
👍 Deleted CdkDataTableModule
| /** boolean value to mark whether the current header is stuck*/ | ||
| isStuck: boolean = false; | ||
| /** Whether the browser support CSS sticky positioning. */ | ||
| private _isStickyPositionSupported: boolean = true; |
There was a problem hiding this comment.
👍 Changed to '_isPositionStickySupported'
| onResize(): void { | ||
| this.defineRestrictionsAndStick(); | ||
| /** | ||
| * If there's already a header being stick when the page is |
@angular/cdk
jelbourn
reviewed
Jul 24, 2017
| this.stickyParent = this.parentRegion != null ? | ||
| this.parentRegion._elementRef.nativeElement : this.element.parentElement; | ||
|
|
||
| let values = window.getComputedStyle(this.element, ''); |
There was a problem hiding this comment.
👍 renamed: values -> headerStyles
| left: values.left, | ||
| bottom: values.bottom, | ||
| width: values.width, | ||
| zIndex: values.zIndex} as CSSStyleDeclaration; |
There was a problem hiding this comment.
Move closing brace to the next line, e.g.
this._originalStyles = {
position: values.position,
// ...
zIndex: values.zIndex,
} as CSSStyleDeclaration;| } | ||
|
|
||
| ngOnDestroy(): void { | ||
| if (this._onScrollSubscription) { |
There was a problem hiding this comment.
You could do
[this._onScrollSubscription, this._onScrollSubscription, this._onResizeSubscription]
.forEach(s => s && s.unsubscribe());There was a problem hiding this comment.
👍 Changed it to '[this._onScrollSubscription, this._onScrollSubscription, this._onResizeSubscription] .forEach(s => s && s.unsubscribe());'
| private _setStrategyAccordingToCompatibility(): void { | ||
| this._isPositionStickySupported = isPositionStickySupported(); | ||
| if (this._isPositionStickySupported) { | ||
| this.element.style.top = '0px'; |
There was a problem hiding this comment.
You should be able to do just '0' instead of '0px'
| if (this._isPositionStickySupported) { | ||
| this.element.style.top = '0px'; | ||
| this.element.style.cssText += 'position: -webkit-sticky; position: sticky; '; | ||
| // TODO add css class with both 'sticky' and '-webkit-sticky' on position |
There was a problem hiding this comment.
Format like TODO(sllethe): ...
| * will be scrolled up with its cdkStickyRegion element. In this way, the sticky header | ||
| * can be changed smoothly when two sticky header meet and the later one need to replace | ||
| * the former one. | ||
| */ |
There was a problem hiding this comment.
How about
/**
* Unsticks the header so that it goes back to scrolling normally.
*
* This should be called when the element reaches the bottom of its cdkStickyRegion so that it
* smoothly scrolls out of view as the next sticky-header moves in.
*/| right: '0', | ||
| left: 'auto', | ||
| bottom: '0', | ||
| width: this._originalStyles.width}; |
There was a problem hiding this comment.
Move closing brace to next line
| * 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. | ||
| */ | ||
| sticker(): void { |
There was a problem hiding this comment.
How about something like _applyStickyPositionStyles? The name sticker doesn't tell you much about what the function does.
There was a problem hiding this comment.
changed name to 'applyStickyPositionStyles'
| if (this.isStuck && | ||
| (currentPosition < this._stickyRegionTop || | ||
| currentPosition > this._stickyRegionBottomThreshold) | ||
| || currentPosition >= this._stickyRegionBottomThreshold) { |
There was a problem hiding this comment.
Should be consistent whether the operators go at the beginning or the end of the line
| } | ||
| } | ||
|
|
||
| defineRestrictionsAndStick(): void { |
There was a problem hiding this comment.
Maybe _updateStickyPositioning?
There was a problem hiding this comment.
👍 Changed to '_updateStickyPositioning'
rename : values -> headerStyles
Move closing brace to the next line
optimized: [this._onScrollSubscription, this._onScrollSubscription, this._onResizeSubscription]
.forEach(s => s && s.unsubscribe());
You should be able to do just '0' instead of '0px'
private _attachEventListeners? Add a description like "Add listeners for events that affect sticky positioning."
optimize doc
Rename 'defineRestrictions' to '_measureStickyRegionBounds'
rename: private _resetElementStyles
let stuckRight: any = this.upperScrollableContainer.getBoundingClientRect().right; chaned 'any' to 'number'
nit
change doc '/** * Unsticks the header so that it goes back to scrolling normally. * * This should be called when the element reaches the bottom of its cdkStickyRegion so that it * smoothly scrolls out of view as the next sticky-header moves in. */'
_unstuckElement -> _unstickElement
rename 'defineRestrictionsAndStick()' to '_updateStickyPositioning()'
sllethe
commented
Jul 24, 2017
| this.stickyParent = this.parentRegion != null ? | ||
| this.parentRegion._elementRef.nativeElement : this.element.parentElement; | ||
|
|
||
| let values = window.getComputedStyle(this.element, ''); |
There was a problem hiding this comment.
👍 renamed: values -> headerStyles
| left: values.left, | ||
| bottom: values.bottom, | ||
| width: values.width, | ||
| zIndex: values.zIndex} as CSSStyleDeclaration; |
| } | ||
|
|
||
| ngOnDestroy(): void { | ||
| if (this._onScrollSubscription) { |
There was a problem hiding this comment.
👍 Changed it to '[this._onScrollSubscription, this._onScrollSubscription, this._onResizeSubscription] .forEach(s => s && s.unsubscribe());'
| private _setStrategyAccordingToCompatibility(): void { | ||
| this._isPositionStickySupported = isPositionStickySupported(); | ||
| if (this._isPositionStickySupported) { | ||
| this.element.style.top = '0px'; |
| if (this._isPositionStickySupported) { | ||
| this.element.style.top = '0px'; | ||
| this.element.style.cssText += 'position: -webkit-sticky; position: sticky; '; | ||
| // TODO add css class with both 'sticky' and '-webkit-sticky' on position |
| * can be changed smoothly when two sticky header meet and the later one need to replace | ||
| * the former one. | ||
| */ | ||
| private _unstuckElement(): void { |
| right: '0', | ||
| left: 'auto', | ||
| bottom: '0', | ||
| width: this._originalStyles.width}; |
| * 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. | ||
| */ | ||
| sticker(): void { |
There was a problem hiding this comment.
changed name to 'applyStickyPositionStyles'
| if (this.isStuck && | ||
| (currentPosition < this._stickyRegionTop || | ||
| currentPosition > this._stickyRegionBottomThreshold) | ||
| || currentPosition >= this._stickyRegionBottomThreshold) { |
| } | ||
| } | ||
|
|
||
| defineRestrictionsAndStick(): void { |
There was a problem hiding this comment.
👍 Changed to '_updateStickyPositioning'
jelbourn
reviewed
Jul 24, 2017
|
LGTM |
|
LGTM :) |
sllethe
added a commit
to sllethe/material2
that referenced
this pull request
Aug 10, 2017
…y header (angular#5858) * add lib files for sticky-header add chose parent add support to 'optional 'cdkStickyRegion' input ' add app-demo for sticky-header fix bugs and deleted unused tag id in HTML files modify * fix some code according to PR review comments * change some format to pass TSlint check * add '_' before private elements * delete @Injectable for StickyHeaderDirective. Because we do not need @Injectable * refine code * encapsulate 'set style for element' * change @input() * Delete 'Observable.fromEvent(this.upperScrollableContainer, 'scroll')' * add const STICK_START_CLASS and STICK_END_CLASS * Add doc for [cdkStickyRegion] and 'unstuckElement()'. Delete 'detach()' function, add its content into 'ngOnDestroy()'. * change 'MdStickyHeaderModule' to 'CdkStickyHeaderModule'; * encapsulate reset css style operation for sticky header. * delete unnecessary gloable variables * delete global variable '_width' * Add doc for 'sticker()' function. explained how it works. * add more doc for 'sticker()', explaining 'isStuck' flag * 2 space for indent * fix * delete sticky-header demo part from this branch * revert firebase file * change code according to comments in PR * revert firbaserc * revert demo-app.ts * revert routes.ts * revert demo-app-module.ts * change * fix the problem of : 'this.stickyParent' might be 'null' * change 'CdkStickyHeaderModule' to 'StickyHeaderModule' * change doc * Change the constructor of 'cdkStickyRegion' to 'constructor(public readonly _elementRef: ElementRef) { }' * Added prefix 'mat-' for CSS class * Delete 'public' before variables * Object.assign isn't supported in IE11; use extendObject from src/lib/core/util. * IE11 will have trouble with `translate3d(0, 0, 0);', change to `translate3d(0px, 0px, 0px);' * Added docs for all variables * extract 'generate CSS style' * created a generateStyleCSS() function, let it be responsible for generating all those CSS styles. * reformat * add debounce to solve 'getBoundingClientRect() cause slow down' problem. * add position:sticky and check whether browser support it. If not , use the naive implementation * removed unused import * Removed unused 'scrollableRegion' and 'parentRegion' * removed commented lines * default public * Add comments about why setting style top and position for iPhone and not IE. And extract detectBrowser() as a new function * format * consider all circumstances of browser. * use "===" instead of '==' * make 'navigator.userAgent.toLocaleLowerCase()' a local variable * optimize * Added comments on const 'STICK_START_CLASS' and 'STICK_END_CLASS'. change their content to cdk-sticky-header-start and cdk-sticky-header-end * Added comments for STICK_START_CLASS and STICK_END_CLASS. * Changed the format of one-line JsDoc * unsubscribe sbscriptions onDestory * Use what modernizr does on compatibility instead of get the browser version directly. * add 'padding' and 'stickyRegionHeight' variables to avoid calling 'getComputedStyle()' too many times (which is expensive). * move docs above @directive * removed the underscore in'_element: ElementRef', * expand 'reg' to 'region' * use 'if (this.isIE)' instead of 'if(this.isIE === true)' * added more newlines between params in 'generateCssStyle()' function to make it easier to understand. * Added reference link to Modernizer in docs of getSupportList() * Deleted "_supportList" variable * renamed 'isIE' to 'isStickyPositionSupported', and removed extra space before Observable * Set debounce time as a const variable * Added docs for 'const DEBOUNCE_TIME: number = 5;' * Changed ' if(this.stickyParent == null)' to ' if(!this.stickyParent)' * Removed the @param and @returns and make sure the types are correct in the function signature in 'generateCssStyle(...)' function * Added docs for `isStickyPositionSupported` variable * changed '+=' to '=' of 'stickyText' in getSupportList() function * nit added " " between 'if' and '(' * nit * Added comments * deleted unused import * change comments * optimize comments * deleted unnecessary global variables(padding and stickyRegionHeight) * Added check whether we are on browser * Array<string> to string[] * test? * try to reopen the old PR * fix after rebase * revert list.ts * test * test 222 * revert demo * revert list.ts second time * Move code to 'src/cdk' * revert 'move code to 'src/cdk'' , it should be done in a new PR * revert * avoid calling 'getComputedStyle()' too many times. * rename as sticky-header.ts * rename sticky-header.ts * imported PlatformModule * Add blank lines between these top-level symbol * make '_isStickyPositionSupported' private * Changed the originalCSS to private and use '{} as CSSStyleDeclaration' instead of ''any. * Rename '_containerStart' to '_stickyRegionTop' * rename * optimize discription for '_stickyRegionBottomThreshold' * private _originalStyles = { position: '', top: '', right: '', left: '', bottom: '', width: '', zIndex: ''}; * Deleted 'generateCssStyle()' and 'getCssNumber()' function * Deleted 'getCssValue()' function * fix CSSStyleDeclaration * change sticky width to 'this.upperScrollableContainer.clientWidth' * fix * nit * Added isPositionStickySupported() to 'src/cdk/platform/featrues.ts' * Added the 'isPositionStickySupported() ' function to src/cdk/platform/features.ts. Consume that function in this component and just always use both the webkit and unprefixed styles. * nit * nit * update doc 'Debounce time in milliseconds for events that affect the sticky positioning (e.g. scroll, resize, touch move). Set as 5 milliseconds which is the highest delay that doesn't drastically affect the positioning adversely.' * changed the doc to '/** z-index to be applied to the sticky header (default is 10). */' * fix tslint error * for comment 'Can you evaluate each method to make sure their accessor privacy is right? E.g. see which functions need to be public, private, static, etc' * Deleted variable 'elemHeight' * Chaned to 'if (!this.stickyParent)' * Simplified Docs for 'sticker()'. * set 'defineRestriction()' function to private * use 'RxChain' * deleted unused 'tableModule' in modules.ts * rename to '_isPositionStickySupported' * Use // for comments, /* */ for docs * @angular/cdk * rename : values -> headerStyles * Move closing brace to the next line * optimized: [this._onScrollSubscription, this._onScrollSubscription, this._onResizeSubscription] .forEach(s => s && s.unsubscribe()); * You should be able to do just '0' instead of '0px' * Format like TODO(sllethe): ... * private _attachEventListeners? Add a description like "Add listeners for events that affect sticky positioning." * optimize doc * Rename 'defineRestrictions' to '_measureStickyRegionBounds' * rename: private _resetElementStyles * let stuckRight: any = this.upperScrollableContainer.getBoundingClientRect().right; chaned 'any' to 'number' * nit * change doc '/** * Unsticks the header so that it goes back to scrolling normally. * * This should be called when the element reaches the bottom of its cdkStickyRegion so that it * smoothly scrolls out of view as the next sticky-header moves in. */' * _unstuckElement -> _unstickElement * rename 'sticker()' to '_applyStickyPositionStyles()' * rename 'defineRestrictionsAndStick()' to '_updateStickyPositioning()'
sllethe
added a commit
to sllethe/material2
that referenced
this pull request
Aug 10, 2017
…y header (angular#5858) * add lib files for sticky-header add chose parent add support to 'optional 'cdkStickyRegion' input ' add app-demo for sticky-header fix bugs and deleted unused tag id in HTML files modify * fix some code according to PR review comments * change some format to pass TSlint check * add '_' before private elements * delete @Injectable for StickyHeaderDirective. Because we do not need @Injectable * refine code * encapsulate 'set style for element' * change @input() * Delete 'Observable.fromEvent(this.upperScrollableContainer, 'scroll')' * add const STICK_START_CLASS and STICK_END_CLASS * Add doc for [cdkStickyRegion] and 'unstuckElement()'. Delete 'detach()' function, add its content into 'ngOnDestroy()'. * change 'MdStickyHeaderModule' to 'CdkStickyHeaderModule'; * encapsulate reset css style operation for sticky header. * delete unnecessary gloable variables * delete global variable '_width' * Add doc for 'sticker()' function. explained how it works. * add more doc for 'sticker()', explaining 'isStuck' flag * 2 space for indent * fix * delete sticky-header demo part from this branch * revert firebase file * change code according to comments in PR * revert firbaserc * revert demo-app.ts * revert routes.ts * revert demo-app-module.ts * change * fix the problem of : 'this.stickyParent' might be 'null' * change 'CdkStickyHeaderModule' to 'StickyHeaderModule' * change doc * Change the constructor of 'cdkStickyRegion' to 'constructor(public readonly _elementRef: ElementRef) { }' * Added prefix 'mat-' for CSS class * Delete 'public' before variables * Object.assign isn't supported in IE11; use extendObject from src/lib/core/util. * IE11 will have trouble with `translate3d(0, 0, 0);', change to `translate3d(0px, 0px, 0px);' * Added docs for all variables * extract 'generate CSS style' * created a generateStyleCSS() function, let it be responsible for generating all those CSS styles. * reformat * add debounce to solve 'getBoundingClientRect() cause slow down' problem. * add position:sticky and check whether browser support it. If not , use the naive implementation * removed unused import * Removed unused 'scrollableRegion' and 'parentRegion' * removed commented lines * default public * Add comments about why setting style top and position for iPhone and not IE. And extract detectBrowser() as a new function * format * consider all circumstances of browser. * use "===" instead of '==' * make 'navigator.userAgent.toLocaleLowerCase()' a local variable * optimize * Added comments on const 'STICK_START_CLASS' and 'STICK_END_CLASS'. change their content to cdk-sticky-header-start and cdk-sticky-header-end * Added comments for STICK_START_CLASS and STICK_END_CLASS. * Changed the format of one-line JsDoc * unsubscribe sbscriptions onDestory * Use what modernizr does on compatibility instead of get the browser version directly. * add 'padding' and 'stickyRegionHeight' variables to avoid calling 'getComputedStyle()' too many times (which is expensive). * move docs above @directive * removed the underscore in'_element: ElementRef', * expand 'reg' to 'region' * use 'if (this.isIE)' instead of 'if(this.isIE === true)' * added more newlines between params in 'generateCssStyle()' function to make it easier to understand. * Added reference link to Modernizer in docs of getSupportList() * Deleted "_supportList" variable * renamed 'isIE' to 'isStickyPositionSupported', and removed extra space before Observable * Set debounce time as a const variable * Added docs for 'const DEBOUNCE_TIME: number = 5;' * Changed ' if(this.stickyParent == null)' to ' if(!this.stickyParent)' * Removed the @param and @returns and make sure the types are correct in the function signature in 'generateCssStyle(...)' function * Added docs for `isStickyPositionSupported` variable * changed '+=' to '=' of 'stickyText' in getSupportList() function * nit added " " between 'if' and '(' * nit * Added comments * deleted unused import * change comments * optimize comments * deleted unnecessary global variables(padding and stickyRegionHeight) * Added check whether we are on browser * Array<string> to string[] * test? * try to reopen the old PR * fix after rebase * revert list.ts * test * test 222 * revert demo * revert list.ts second time * Move code to 'src/cdk' * revert 'move code to 'src/cdk'' , it should be done in a new PR * revert * avoid calling 'getComputedStyle()' too many times. * rename as sticky-header.ts * rename sticky-header.ts * imported PlatformModule * Add blank lines between these top-level symbol * make '_isStickyPositionSupported' private * Changed the originalCSS to private and use '{} as CSSStyleDeclaration' instead of ''any. * Rename '_containerStart' to '_stickyRegionTop' * rename * optimize discription for '_stickyRegionBottomThreshold' * private _originalStyles = { position: '', top: '', right: '', left: '', bottom: '', width: '', zIndex: ''}; * Deleted 'generateCssStyle()' and 'getCssNumber()' function * Deleted 'getCssValue()' function * fix CSSStyleDeclaration * change sticky width to 'this.upperScrollableContainer.clientWidth' * fix * nit * Added isPositionStickySupported() to 'src/cdk/platform/featrues.ts' * Added the 'isPositionStickySupported() ' function to src/cdk/platform/features.ts. Consume that function in this component and just always use both the webkit and unprefixed styles. * nit * nit * update doc 'Debounce time in milliseconds for events that affect the sticky positioning (e.g. scroll, resize, touch move). Set as 5 milliseconds which is the highest delay that doesn't drastically affect the positioning adversely.' * changed the doc to '/** z-index to be applied to the sticky header (default is 10). */' * fix tslint error * for comment 'Can you evaluate each method to make sure their accessor privacy is right? E.g. see which functions need to be public, private, static, etc' * Deleted variable 'elemHeight' * Chaned to 'if (!this.stickyParent)' * Simplified Docs for 'sticker()'. * set 'defineRestriction()' function to private * use 'RxChain' * deleted unused 'tableModule' in modules.ts * rename to '_isPositionStickySupported' * Use // for comments, /* */ for docs * @angular/cdk * rename : values -> headerStyles * Move closing brace to the next line * optimized: [this._onScrollSubscription, this._onScrollSubscription, this._onResizeSubscription] .forEach(s => s && s.unsubscribe()); * You should be able to do just '0' instead of '0px' * Format like TODO(sllethe): ... * private _attachEventListeners? Add a description like "Add listeners for events that affect sticky positioning." * optimize doc * Rename 'defineRestrictions' to '_measureStickyRegionBounds' * rename: private _resetElementStyles * let stuckRight: any = this.upperScrollableContainer.getBoundingClientRect().right; chaned 'any' to 'number' * nit * change doc '/** * Unsticks the header so that it goes back to scrolling normally. * * This should be called when the element reaches the bottom of its cdkStickyRegion so that it * smoothly scrolls out of view as the next sticky-header moves in. */' * _unstuckElement -> _unstickElement * rename 'sticker()' to '_applyStickyPositionStyles()' * rename 'defineRestrictionsAndStick()' to '_updateStickyPositioning()'
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I closed the old PR (feat(sticky-header): Initial version of sticky header #5175 : #5175) by mistake and it can not be reopened. So this is the new PR...