fix(core): improve use of breakpoint priorities #946

ThomasBurleson · 2018-12-20T18:01:35Z

Use breakpoint priority as the only sorting/scanning mechanism;
used to ensure correct MediaChange event notifications.

prioritize breakpoints: non-overlaps hightest, lt- lowest
consistently sort breakpoints ascending by priority
- highest priority === smallest range
- remove hackery with reverse(), etc.
memoize BreakPointRegistry findBy lookups
fix MatchMedia::observe() to support lazy breakpoint registration
fix fragile logic in MediaMarshaller
- fix breakpoint registration usage
- clarify update/clear builder function callbacks
fix MediaObserver breakpoint registration usage

googlebot · 2018-12-20T18:01:37Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ThomasBurleson · 2018-12-20T18:02:42Z

This includes the PR to remove deprecated APIs; to ensure optimizations work as desired.

CaerusKaru

This is stellar work; I'm especially amazed that more tests didn't need to be patched as a result. Just some minor cosmetic changes; functionally everything looks great!

.firebase/hosting.ZGlzdC9wYWNrYWdlcy9kZW1vLWFwcA.cache

src/lib/core/breakpoints/break-point-registry.ts

src/lib/core/breakpoints/breakpoint-tools.ts

src/lib/server/server-provider.ts

src/lib/core/match-media/match-media.ts

CaerusKaru

Some things missed the first time around. If there's a reason you're keeping them as-is, please provide.

src/lib/core/breakpoints/break-point-registry.ts

src/lib/core/match-media/match-media.ts

CaerusKaru · 2018-12-20T19:22:49Z

src/lib/core/match-media/match-media.ts

@@ -99,7 +118,7 @@ export class MatchMedia {
 * Private global registry for all dynamically-created, injected style tags
 * @see prepare(query)
 */
-const ALL_STYLES: {[key: string]: any} = {};
+const ALL_STYLES: { [key: string]: any } = {};


Still no spaces (per style guide)

src/lib/core/media-marshaller/media-marshaller.ts

CaerusKaru · 2018-12-20T19:24:18Z

src/lib/core/media-marshaller/media-marshaller.ts

-      if (builder) {
-        builder();
+      const clearFn: ClearCallback = builders.get(key) as ClearCallback;
+      if (!!clearFn) {


Again, if(clearFn !== undefined)

CaerusKaru · 2018-12-20T19:24:28Z

src/lib/core/media-marshaller/media-marshaller.ts

-      if (builder) {
-        builder(value);
+      const updateFn: UpdateCallback = builders.get(key) as UpdateCallback;
+      if (!!updateFn) {


Again, if (updateFn !== undefined)

CaerusKaru · 2018-12-20T19:24:52Z

src/lib/core/media-observer/media-observer.ts

-  private _registerBreakPoints() {
-    const queries = this.breakpoints.sortedItems.map(bp => bp.mediaQuery);
-    this.mediaWatcher.registerQuery(queries);
+  private watchActivations() {


Again, add return type

CaerusKaru · 2018-12-21T00:10:35Z

This is also supposed to fix #648 and #426, please add an explicit test case for the former (the latter may be trickier).

CaerusKaru · 2018-12-21T19:26:32Z

src/lib/core/breakpoints/break-point-registry.ts

-    let nonOverlaps = this._registry.filter(it => it.overlapping !== true);
-
-    return [...overlaps, ...nonOverlaps];
+  constructor(@Inject(BREAKPOINTS) list: BreakPoint[]) {


Here's a thought: the items remain static, so instead of memoizing, why not just prime it at startup by creating two maps on init, one for aliases, one for mediaqueries?

Use breakpoint priority as the only sorting/scanning mechanism; used to ensure correct MediaChange event notifications. * prioritize breakpoints: non-overlaps hightest, lt- lowest * consistently sort breakpoints ascending by priority * highest priority === smallest range * remove hackery with reverse(), etc. * memoize BreakPointRegistry findBy lookups * fix MatchMedia::observe() to support lazy breakpoint registration * fix fragile logic in MediaMarshaller * fix breakpoint registration usage * clarify update/clear builder function callbacks * fix MediaObserver breakpoint registration usage Fixes #648, Fixes #426

googlebot · 2018-12-22T19:34:43Z

CLAs look good, thanks!

angular-automatic-lock-bot · 2019-09-06T00:04:38Z

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.}

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Dec 20, 2018

CaerusKaru suggested changes Dec 20, 2018

View reviewed changes

ThomasBurleson force-pushed the thomas/fix-bp-priority branch 2 times, most recently from d4c9619 to 6e66ab6 Compare December 20, 2018 19:14

CaerusKaru suggested changes Dec 20, 2018

View reviewed changes

CaerusKaru added the pr: needs cleanup/tests label Dec 20, 2018

CaerusKaru added this to the 7.0.0-beta.23 milestone Dec 21, 2018

CaerusKaru self-assigned this Dec 21, 2018

CaerusKaru reviewed Dec 21, 2018

View reviewed changes

ThomasBurleson mentioned this pull request Dec 22, 2018

feat(core): add print support with mediaQuery override #954

Merged

ThomasBurleson force-pushed the thomas/fix-bp-priority branch from 6e66ab6 to 350e753 Compare December 22, 2018 19:34

googlebot added cla: yes and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Dec 22, 2018

ThomasBurleson closed this Dec 22, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): improve use of breakpoint priorities #946

fix(core): improve use of breakpoint priorities #946

ThomasBurleson commented Dec 20, 2018

googlebot commented Dec 20, 2018

ThomasBurleson commented Dec 20, 2018

CaerusKaru left a comment

CaerusKaru left a comment

CaerusKaru Dec 20, 2018

CaerusKaru Dec 20, 2018

CaerusKaru Dec 20, 2018

CaerusKaru Dec 20, 2018

CaerusKaru commented Dec 21, 2018

CaerusKaru Dec 21, 2018

googlebot commented Dec 22, 2018

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

fix(core): improve use of breakpoint priorities #946

fix(core): improve use of breakpoint priorities #946

Conversation

ThomasBurleson commented Dec 20, 2018

googlebot commented Dec 20, 2018

ThomasBurleson commented Dec 20, 2018

CaerusKaru left a comment

Choose a reason for hiding this comment

CaerusKaru left a comment

Choose a reason for hiding this comment

CaerusKaru Dec 20, 2018

Choose a reason for hiding this comment

CaerusKaru Dec 20, 2018

Choose a reason for hiding this comment

CaerusKaru Dec 20, 2018

Choose a reason for hiding this comment

CaerusKaru Dec 20, 2018

Choose a reason for hiding this comment

CaerusKaru commented Dec 21, 2018

CaerusKaru Dec 21, 2018

Choose a reason for hiding this comment

googlebot commented Dec 22, 2018

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