-
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): startup should propagate lastReplay value properly #313
Conversation
a58bcb4
to
d9177cb
Compare
277654d
to
314bc9b
Compare
@@ -31,7 +31,9 @@ export class MediaQueryStatus implements OnDestroy { | |||
private _watcher : Subscription; | |||
activeMediaQuery : string; | |||
|
|||
constructor(media$ : ObservableMedia) { this.watchMediaQueries(media$); } | |||
constructor(media$ : ObservableMedia) { | |||
this.watchMediaQueries(media$); |
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.
What's the $
for in the variable name? If it's for private but cannot be marked as such for Angular reasons, we use an underscore to preface the variable
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.
The $
suffix is a community convention to indicate a variable reference that is actually an observable to future value(S)... originally seen in use by Andre Stalz (2 yrs ago).
I believe this is used consistently throughout the library.
@@ -30,6 +30,20 @@ export class BreakPointRegistry { | |||
} | |||
|
|||
/** | |||
* Accessor to sorted list used for |
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.
Can combine the two lines into 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.
So I do not mind these comments. It is my hope however that reviewers will focus mostly on content and less on format.
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.
Tried to give focus to both equally :) My understanding is that we'll want to review this library with the same rigor we give to Material, and we're often strict about even the small things just to maintain quality (even in comments). Apologies if it seems pedantic, just wanting to make sure I'm fair to all PRs
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.
Understood. Thanks for helping review these. Much appreciated.
* so the non-overlaps will trigger the MatchMedia:BehaviorSubject last! | ||
* And the largest, non-overlap, matching breakpoint should be the lastReplay value | ||
*/ | ||
get sortedItems(): BreakPoint[ ] { |
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.
Remove space between [ ]
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.
k
* And the largest, non-overlap, matching breakpoint should be the lastReplay value | ||
*/ | ||
get sortedItems(): BreakPoint[ ] { | ||
let overlaps = this._registry.filter(it=>it.overlapping === true); |
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.
Add spaces around =>
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.
Yes. Will fix with style Linter tool.
let current: MediaChange, activated; | ||
let query1 = "screen and (min-width: 610px) and (max-width: 620px)"; | ||
let query2 = "(min-width: 730px) and (max-width: 950px)"; | ||
|
||
matchMedia.registerQuery(query1); | ||
matchMedia.registerQuery(query2); | ||
matchMedia.registerQuery([query1,query2]); |
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.
Add space after ,
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.
k.
src/lib/media-query/match-media.ts
Outdated
this._source.next(change); | ||
}); | ||
}; | ||
registerQuery(mediaQuery: string | Array<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.
use 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.
k.
314bc9b
to
062d342
Compare
ObservableMedia only dispatches notifications for activated, non-overlapping breakpoints. If the MatchMedia lastReplay value is an *overlapping* breakpoint (e.g. `lt-md`, `gt-lg`) then that value will be filtered by ObservableMedia and not be emitted to subscribers. * MatchMedia breakpoints registration was not correct * overlapping breakpoints were registered in the wrong order * non-overlapping breakpoints should be registered last; so the BehaviorSubject's last replay value should be an non-overlapping breakpoint range. * Optimize stylesheet injection to group `n` mediaQuerys in a single stylesheet > See working plunker: https://plnkr.co/edit/yylQr2IdbGy2Yr00srrN?p=preview Fixes #245, #275, #303
062d342
to
9cd1514
Compare
Updated. Thx @andrewseguin. |
Please re-check this issue. I think we still have problem when used with |
@bogacg - Would you mind opening a new issue and perhaps provide a StackBlitz or zip that demonstrates this issue ?
|
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. |
ObservableMedia only dispatches notifications for activated, non-overlapping breakpoints.
If the MatchMedia lastReplay value is an overlapping breakpoint
(e.g.
lt-md
,gt-lg
) then that value will be filtered by ObservableMediaand not be emitted to subscribers.
n
mediaQuerys in a single stylesheetFixes #245, #275, #303.