-
Notifications
You must be signed in to change notification settings - Fork 773
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
fix(ObservableMedia): properly announce 'xs' activation at startup #396
Conversation
@crisbeto - would you mind reviewing this one ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few nitpicks, however I didn't find a new unit test for the fix to #388.
LICENSE
Outdated
@@ -10,7 +10,7 @@ copies of the Software, and to permit persons to whom the Software is | |||
furnished to do so, subject to the following conditions: | |||
|
|||
The above copyright notice and this permission notice shall be included in | |||
all copies or substantial portions of the Software. | |||
all copies or substantial portions of the Software. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you added an extra space in here.
src/lib/api/core/base.ts
Outdated
@@ -130,7 +134,7 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { | |||
* and optional restore it when the mediaQueries deactivate | |||
*/ | |||
protected _getDisplayStyle(source?: HTMLElement): string { | |||
let element: HTMLElement = source || this._elementRef.nativeElement; | |||
let element: HTMLElement = source || this.nativeElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing element = source || this.nativeElement
, you can change the method signature to be protected _getDisplayStyle(source: = this.nativeElement): string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know you could initialize with a dynamic property:
protected _getDisplayStyle(source: HTMLElement = this.nativeElement): string {
so cool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@crisbeto - note sure if you default value solves all the scenarios. Your solution essential does this:
if (source === void 0) { source = this.nativeElement; }
return lookupStyle(source, 'display');
My solution will account for calls like _getDisplayStyle(null)
where your solution will fail with that.
BaseFxDirective.prototype._getDisplayStyle = function (source) {
if (source === void 0) { source = this.nativeElement; }
return lookupStyle(source || this.nativeElement, 'display');
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, didn't know that scenario had to be handled.
src/lib/api/core/base.ts
Outdated
@@ -161,7 +165,7 @@ export abstract class BaseFxDirective implements OnDestroy, OnChanges { | |||
protected _applyStyleToElement(style: StyleDefinition, | |||
value?: string | number, | |||
nativeElement?: any) { | |||
let element = nativeElement || this._elementRef.nativeElement; | |||
let element = nativeElement || this.nativeElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment above regarding why let element = nativeElement || this.nativeElement
is better than a default parameter value.
return { | ||
pass: allPassed, | ||
get message() { | ||
const expectedValueStr = (typeof styles === 'string') ? styles : JSON.stringify(styles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make the output of this a little more readable, you can pass in the indentation level to JSON.stringify
: JSON.stringify(styles, null, 2)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
6c2b448
to
7b64345
Compare
@crisbeto - I just added a unit tests for XS startup activation announcements. |
7b64345
to
1a4ac6c
Compare
MatchMedia was not properly announcing XS activations for applications with startup viewports < 600px. Improve the specificity of the 'xs' media query enables proper MatchMedia notifications at startup. Fixes #388.
1a4ac6c
to
eeea8fb
Compare
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. |
MatchMedia was not properly announcing XS activations for applications with startup viewports < 600px.
Improve the specificity of the 'xs' media query enables proper MatchMedia notifications at startup.
Fixes #388.