fix(core): improve use of breakpoint priorities

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
angular · Dec 28, 2018 · 6367fa9 · 6367fa9
1 parent c922ae3
commit 6367fa9
Show file tree

Hide file tree

Showing 17 changed files with 227 additions and 179 deletions.
diff --git a/.gitignore b/.gitignore
@@ -17,6 +17,7 @@ node_modules
 
 # misc
 .DS_Store
+/.firebase
 /.sass-cache
 /connect.lock
 /coverage/*

diff --git a/src/lib/core/breakpoints/break-point-registry.ts b/src/lib/core/breakpoints/break-point-registry.ts
@@ -9,7 +9,9 @@ import {Injectable, Inject} from '@angular/core';
 
 import {BreakPoint} from './break-point';
 import {BREAKPOINTS} from './break-points-token';
+import {sortAscendingPriority} from './breakpoint-tools';
 
+type OptionalBreakPoint = BreakPoint | null;
 
 /**
  * Registry of 1..n MediaQuery breakpoint ranges
@@ -18,55 +20,36 @@ import {BREAKPOINTS} from './break-points-token';
  */
 @Injectable({providedIn: 'root'})
 export class BreakPointRegistry {
+  readonly items: BreakPoint[];
 
-  constructor(@Inject(BREAKPOINTS) private _registry: BreakPoint[]) {
-  }
-
-  /**
-   * Accessor to raw list
-   */
-  get items(): BreakPoint[] {
-    return [...this._registry];
-  }
-
-  /**
-   * Accessor to sorted list used for registration with matchMedia API
-   *
-   * NOTE: During breakpoint registration, we want to register the overlaps FIRST
-   *       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[] {
-    let overlaps = this._registry.filter(it => it.overlapping === true);
-    let nonOverlaps = this._registry.filter(it => it.overlapping !== true);
-
-    return [...overlaps, ...nonOverlaps];
+  constructor(@Inject(BREAKPOINTS) list: BreakPoint[]) {
+    this.items = [...list].sort(sortAscendingPriority);
   }
 
   /**
    * Search breakpoints by alias (e.g. gt-xs)
    */
-  findByAlias(alias: string): BreakPoint | null {
-    return this._registry.find(bp => bp.alias == alias) || null;
+  findByAlias(alias: string): OptionalBreakPoint {
+    return this.findWithPredicate(alias, (bp) => bp.alias == alias);
   }
 
-  findByQuery(query: string): BreakPoint | null {
-    return this._registry.find(bp => bp.mediaQuery == query) || null;
+  findByQuery(query: string): OptionalBreakPoint {
+    return this.findWithPredicate(query, (bp) => bp.mediaQuery == query);
   }
 
   /**
    * Get all the breakpoints whose ranges could overlapping `normal` ranges;
    * e.g. gt-sm overlaps md, lg, and xl
    */
   get overlappings(): BreakPoint[] {
-    return this._registry.filter(it => it.overlapping == true);
+    return this.items.filter(it => it.overlapping == true);
   }
 
   /**
    * Get list of all registered (non-empty) breakpoint aliases
    */
   get aliases(): string[] {
-    return this._registry.map(it => it.alias);
+    return this.items.map(it => it.alias);
   }
 
   /**
@@ -75,6 +58,26 @@ export class BreakPointRegistry {
    * for property layoutGtSM.
    */
   get suffixes(): string[] {
-    return this._registry.map(it => !!it.suffix ? it.suffix : '');
+    return this.items.map(it => !!it.suffix ? it.suffix : '');
   }
+
+  /**
+   * Memoized lookup using custom predicate function
+   */
+  private findWithPredicate(key: string,
+      searchFn: (bp: BreakPoint) => boolean): OptionalBreakPoint {
+    let response = this.findByMap.get(key);
+    if (!response) {
+      response = this.items.find(searchFn) || null;
+      this.findByMap.set(key, response);
+    }
+    return response || null;
+
+  }
+
+  /**
+   * Memoized BreakPoint Lookups
+   */
+  private readonly findByMap = new Map<String, OptionalBreakPoint>();
 }
+
diff --git a/src/lib/core/breakpoints/break-point.ts b/src/lib/core/breakpoints/break-point.ts
@@ -9,7 +9,6 @@ export interface BreakPoint {
   mediaQuery: string;
   alias: string;
   suffix?: string;
-  overlapping?: boolean;
-  // The priority of the individual breakpoint when overlapping another breakpoint
-  priority?: number;
+  overlapping?: boolean;  // Does this range overlap with any other ranges
+  priority?: number;      // determine order of activation reporting: higher is last reported
 }
diff --git a/src/lib/core/breakpoints/breakpoint-tools.ts b/src/lib/core/breakpoints/breakpoint-tools.ts
@@ -65,8 +65,14 @@ export function mergeByAlias(defaults: BreakPoint[], custom: BreakPoint[] = []):
 }
 
 /** HOF to sort the breakpoints by priority */
-export function prioritySort(a: BreakPoint, b: BreakPoint): number {
+export function sortDescendingPriority(a: BreakPoint, b: BreakPoint): number {
   const priorityA = a.priority || 0;
   const priorityB = b.priority || 0;
   return priorityB - priorityA;
 }
+
+export function sortAscendingPriority(a: BreakPoint, b: BreakPoint): number {
+  const pA = a.priority || 0;
+  const pB = b.priority || 0;
+  return pA - pB;
+}
diff --git a/src/lib/core/breakpoints/data/break-points.spec.ts b/src/lib/core/breakpoints/data/break-points.spec.ts
@@ -6,6 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {TestBed, inject, async} from '@angular/core/testing';
+import {sortDescendingPriority} from '../breakpoint-tools';
 
 import {BreakPoint} from '../break-point';
 import {BREAKPOINTS} from '../break-points-token';
@@ -22,13 +23,13 @@ describe('break-point-provider', () => {
       });
     });
     beforeEach(async(inject([BREAKPOINTS], (bps: BreakPoint[]) => {
-      breakPoints = bps;
+      breakPoints = [...bps].sort(sortDescendingPriority);
     })));
 
     it('has the standard breakpoints', () => {
       expect(breakPoints.length).toEqual(DEFAULT_BREAKPOINTS.length);
       expect(breakPoints[0].alias).toEqual('xs');
-      expect(breakPoints[breakPoints.length - 1].alias).toEqual('xl');
+      expect(breakPoints[breakPoints.length - 1].alias).toEqual('gt-xs');
     });
   });
 

diff --git a/src/lib/core/breakpoints/data/break-points.ts b/src/lib/core/breakpoints/data/break-points.ts
@@ -7,83 +7,81 @@
  */
 import {BreakPoint} from '../break-point';
 
-export const RESPONSIVE_ALIASES = [
-  'xs', 'gt-xs', 'sm', 'gt-sm', 'md', 'gt-md', 'lg', 'gt-lg', 'xl'
-];
-
+/**
+ * NOTE: Smaller ranges have HIGHER priority since the match is more specific
+ */
 export const DEFAULT_BREAKPOINTS: BreakPoint[] = [
   {
     alias: 'xs',
-    mediaQuery: '(min-width: 0px) and (max-width: 599px)',
-    priority: 100,
+    mediaQuery: 'screen and (min-width: 0px) and (max-width: 599px)',
+    priority: 1000,
   },
   {
-    alias: 'gt-xs',
-    overlapping: true,
-    mediaQuery: '(min-width: 600px)',
-    priority: 7,
+    alias: 'sm',
+    mediaQuery: 'screen and (min-width: 600px) and (max-width: 959px)',
+    priority: 900,
   },
   {
-    alias: 'lt-sm',
-    overlapping: true,
-    mediaQuery: '(max-width: 599px)',
-    priority: 10,
+    alias: 'md',
+    mediaQuery: 'screen and (min-width: 960px) and (max-width: 1279px)',
+    priority: 800,
   },
   {
-    alias: 'sm',
-    mediaQuery: '(min-width: 600px) and (max-width: 959px)',
-    priority: 100,
+    alias: 'lg',
+    mediaQuery: 'screen and (min-width: 1280px) and (max-width: 1919px)',
+    priority: 700,
   },
   {
-    alias: 'gt-sm',
-    overlapping: true,
-    mediaQuery: '(min-width: 960px)',
-    priority: 8,
+    alias: 'xl',
+    mediaQuery: 'screen and (min-width: 1920px) and (max-width: 5000px)',
+    priority: 600,
   },
   {
-    alias: 'lt-md',
+    alias: 'lt-sm',
     overlapping: true,
-    mediaQuery: '(max-width: 959px)',
-    priority: 9,
+    mediaQuery: 'screen and (max-width: 599px)',
+    priority: 950,
   },
   {
-    alias: 'md',
-    mediaQuery: '(min-width: 960px) and (max-width: 1279px)',
-    priority: 100,
-  },
-  {
-    alias: 'gt-md',
+    alias: 'lt-md',
     overlapping: true,
-    mediaQuery: '(min-width: 1280px)',
-    priority: 9,
+    mediaQuery: 'screen and (max-width: 959px)',
+    priority: 850,
   },
   {
     alias: 'lt-lg',
     overlapping: true,
-    mediaQuery: '(max-width: 1279px)',
-    priority: 8,
+    mediaQuery: 'screen and (max-width: 1279px)',
+    priority: 750,
   },
   {
-    alias: 'lg',
-    mediaQuery: '(min-width: 1280px) and (max-width: 1919px)',
-    priority: 100,
+    alias: 'lt-xl',
+    overlapping: true,
+    priority: 650,
+    mediaQuery: 'screen and (max-width: 1919px)',
   },
   {
-    alias: 'gt-lg',
+    alias: 'gt-xs',
     overlapping: true,
-    mediaQuery: '(min-width: 1920px)',
-    priority: 10,
+    mediaQuery: 'screen and (min-width: 600px)',
+    priority: -950,
   },
   {
-    alias: 'lt-xl',
+    alias: 'gt-sm',
+    overlapping: true,
+    mediaQuery: 'screen and (min-width: 960px)',
+    priority: -850,
+  }, {
+    alias: 'gt-md',
     overlapping: true,
-    mediaQuery: '(max-width: 1919px)',
-    priority: 7,
+    mediaQuery: 'screen and (min-width: 1280px)',
+    priority: -750,
   },
   {
-    alias: 'xl',
-    mediaQuery: '(min-width: 1920px) and (max-width: 5000px)',
-    priority: 100,
+    alias: 'gt-lg',
+    overlapping: true,
+    mediaQuery: 'screen and (min-width: 1920px)',
+    priority: -650,
   }
 ];
 
diff --git a/src/lib/core/breakpoints/data/orientation-break-points.ts b/src/lib/core/breakpoints/data/orientation-break-points.ts
@@ -36,15 +36,15 @@ export const ScreenTypes = {
  * Extended Breakpoints for handset/tablets with landscape or portrait orientations
  */
 export const ORIENTATION_BREAKPOINTS : BreakPoint[] = [
-  {'alias': 'handset',            'mediaQuery': ScreenTypes.HANDSET},
-  {'alias': 'handset.landscape',  'mediaQuery': ScreenTypes.HANDSET_LANDSCAPE},
-  {'alias': 'handset.portrait',   'mediaQuery': ScreenTypes.HANDSET_PORTRAIT},
+  {'alias': 'handset',            priority: 10000, 'mediaQuery': ScreenTypes.HANDSET},
+  {'alias': 'handset.landscape',  priority: 10000, 'mediaQuery': ScreenTypes.HANDSET_LANDSCAPE},
+  {'alias': 'handset.portrait',   priority: 10000, 'mediaQuery': ScreenTypes.HANDSET_PORTRAIT},
 
-  {'alias': 'tablet',             'mediaQuery': ScreenTypes.TABLET},
-  {'alias': 'tablet.landscape',   'mediaQuery': ScreenTypes.TABLET},
-  {'alias': 'tablet.portrait',    'mediaQuery': ScreenTypes.TABLET_PORTRAIT},
+  {'alias': 'tablet',             priority: 8000, 'mediaQuery': ScreenTypes.TABLET},
+  {'alias': 'tablet.landscape',   priority: 8000, 'mediaQuery': ScreenTypes.TABLET},
+  {'alias': 'tablet.portrait',    priority: 8000, 'mediaQuery': ScreenTypes.TABLET_PORTRAIT},
 
-  {'alias': 'web',                'mediaQuery': ScreenTypes.WEB, overlapping : true },
-  {'alias': 'web.landscape',      'mediaQuery': ScreenTypes.WEB_LANDSCAPE, overlapping : true },
-  {'alias': 'web.portrait',       'mediaQuery': ScreenTypes.WEB_PORTRAIT, overlapping : true }
+  {'alias': 'web',                priority: 9000, 'mediaQuery': ScreenTypes.WEB, overlapping : true },
+  {'alias': 'web.landscape',      priority: 9000, 'mediaQuery': ScreenTypes.WEB_LANDSCAPE, overlapping : true },
+  {'alias': 'web.portrait',       priority: 9000, 'mediaQuery': ScreenTypes.WEB_PORTRAIT, overlapping : true }
 ];
diff --git a/src/lib/core/breakpoints/index.ts b/src/lib/core/breakpoints/index.ts
@@ -12,4 +12,8 @@ export * from './data/orientation-break-points';
 export * from './break-point';
 export * from './break-point-registry';
 export * from './break-points-token';
-export {prioritySort} from './breakpoint-tools';
+
+export {
+  sortDescendingPriority,
+  sortAscendingPriority
+} from './breakpoint-tools';
diff --git a/src/lib/core/match-media/match-media.spec.ts b/src/lib/core/match-media/match-media.spec.ts
@@ -8,7 +8,6 @@
 import {TestBed, inject, async} from '@angular/core/testing';
 
 import {MediaChange} from '../media-change';
-import {BreakPoint} from '../breakpoints/break-point';
 import {MockMatchMedia, MockMatchMediaProvider} from './mock/mock-match-media';
 import {BreakPointRegistry} from '../breakpoints/break-point-registry';
 import {MatchMedia} from './match-media';
@@ -51,11 +50,8 @@ describe('match-media', () => {
     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);
-
-    let media$ = matchMedia.observe();
-    let subscription = media$.subscribe((change: MediaChange) => {
+    const queries = [query1, query2];
+    let subscription = matchMedia.observe(queries).subscribe((change: MediaChange) => {
       current = change;
     });
 
@@ -83,7 +79,7 @@ describe('match-media', () => {
 
     matchMedia.registerQuery([query1, query2]);
 
-    let subscription = matchMedia.observe(query1).subscribe((change: MediaChange) => {
+    let subscription = matchMedia.observe([query1]).subscribe((change: MediaChange) => {
       current = change;
     });
 
@@ -128,11 +124,6 @@ describe('match-media-observable', () => {
       matchMedia = _matchMedia;      // inject only to manually activate mediaQuery ranges
       breakPoints = _breakPoints;
       mediaObserver = _mediaObserver;
-
-      // Quick register all breakpoint mediaQueries
-      breakPoints.items.forEach((bp: BreakPoint) => {
-        matchMedia.observe(bp.mediaQuery);
-      });
     })));
   afterEach(() => {
     matchMedia.clearAll();
@@ -206,6 +197,8 @@ describe('match-media-observable', () => {
   it('ignores mediaQuery de-activations', () => {
     let activationCount = 0;
     let deactivationCount = 0;
+
+    mediaObserver.filterOverlaps = false;
     let subscription = mediaObserver.media$.subscribe((change: MediaChange) => {
       if (change.matches) {
         ++activationCount;