feat(breakpoints): Create breakpoint management system #6772

josephperrott · 2017-08-31T21:01:18Z

fxck · 2017-09-02T06:34:54Z

Doesn't flex-layout have something like this?

jelbourn · 2017-09-05T17:22:04Z

Flex-layout is going to change to consume what's in the cdk

jelbourn · 2017-09-05T17:23:24Z

src/cdk/layout/breakpoints-observer.md

@@ -0,0 +1,26 @@
+### BreakpointsModule
+
+When including the CDK's `BreakpointsModule`, components can inject `BreakpointsManager` to request


LayoutModule, BreakpointObserver

jelbourn · 2017-09-05T17:25:18Z

src/cdk/layout/breakpoints-observer.spec.ts

+      mediaMatcher = mm;
+    }));
+
+  afterEach(inject([MediaMatcher], (_mediaMatcher: FakeMediaMatcher) => {


I don't think you need to inject here vs. using the mediaMatcher you already have

jelbourn · 2017-09-05T18:20:30Z

src/cdk/layout/breakpoints-observer.spec.ts

+import {async, TestBed, inject} from '@angular/core/testing';
+import {Injectable} from '@angular/core';
+
+describe('BreakpointObserver', () => {


None of these tests cover the case when mediaMatcher doesn't match; we could add a method to the fake like setNoMatch(...)

jelbourn · 2017-09-05T18:22:07Z

src/cdk/layout/breakpoints-observer.ts

+
+export type BreakpointQuery = string | string[];
+
+export interface BreakpointState {


Add JsDoc description for interfaces / classes

jelbourn · 2017-09-05T18:24:05Z

src/cdk/layout/breakpoints-observer.ts

+
+@Injectable()
+export class BreakpointObserver implements OnDestroy {
+  // A map of all media queries currently being listened for.


Use JsDoc-style (/** */) comments for member descriptions

jelbourn · 2017-09-05T22:07:48Z

src/cdk/layout/media-matcher.ts

+/**
+ * Global registry for all dynamically-created, injected style tags.
+ */
+let styleElementForWebkitCompatibility: {[key: string]: HTMLStyleElement} = {};


Why not make this a Map<string, HTMLStyleElement>?

Also const

jelbourn · 2017-09-05T22:09:00Z

src/cdk/layout/media-matcher.ts

+
+  constructor(private platform: Platform) {
+    this._matchMedia = this.platform.isBrowser ?
+      (<any>window).matchMedia.bind((<any>window)) :


Why does this need bind?

We are unable to bind the window.matchMedia method as a property of our custom object as it results in an illegal invocation.
So we instead bind it to window so that its execution scope is always as expected

Add a comment mentioning this?

jelbourn · 2017-09-05T22:10:51Z

src/cdk/layout/media-matcher.ts

+
+  constructor(private platform: Platform) {
+    this._matchMedia = this.platform.isBrowser ?
+      (<any>window).matchMedia.bind((<any>window)) :


Why does this need the <any> coercion? The typings look like they have matchMedia present

jelbourn · 2017-09-05T22:11:56Z

src/cdk/layout/media-matcher.ts

+  constructor(private platform: Platform) {
+    this._matchMedia = this.platform.isBrowser ?
+      (<any>window).matchMedia.bind((<any>window)) :
+      (query: string) => <MediaQueryList>{


Extract this into a const outside the class like noopMatchMedia? That way it can have it's own description like

/** No-op matchMedia replacement for non-browser platforms that... */

jelbourn · 2017-09-05T22:12:43Z

src/cdk/layout/media-matcher.ts

+ */
+function createEmptyStyleRule(query: string) {
+  if (!styleElementForWebkitCompatibility[query]) {
+    try {


Why the try-catch? Especially when the catch still logs the error

This is actually a hold over from @ThomasBurleson 's original implementation. My understanding is the try-catch is intended to allow the service to fail gracefully and not cause an entire halt.

My stance is that any try-catch need a comment explaining which error it is catching and why it's something that has to be caught instead of prevented in the first place

@ThomasBurleson do you remember if there was a specific reason for using try around creating a style element?

jelbourn · 2017-09-06T21:22:43Z

src/cdk/layout/breakpoints-observer.ts

+  /** Whether the query currently is matched. */
+  isMatched(value: string | string[]): boolean {
+    let queries = coerceArray(value);
+    return queries.some((mediaQuery: string) => {


The : string shouldn't be necessary since it is inferred from the array. It's also acceptable to use a single-character variable for concise lambdas like this:

return queries.some(q => this._registerQuery(q).mql.matches);

jelbourn · 2017-09-06T21:24:01Z

src/cdk/layout/breakpoints-observer.ts

+    let observables = queries.map(query => this._registerQuery(query).observable);
+
+    return combineLatest(observables, (a: BreakpointState, b: BreakpointState) => {
+      return <BreakpointState>{


Still has <BreakpointState>

jelbourn · 2017-09-06T21:25:21Z

src/cdk/layout/breakpoints-observer.ts

+       * webapis-media-query.js file alongside the zone.js file.  Additionally, some browsers do not
+       * have MediaQueryList inherit from EventTarget, which causes inconsistencies in how Zone.js
+       * patches it.
+       */


Use //-style comments for explanations / details (with /** */ for class/member/variable descriptions)

jelbourn · 2017-09-06T21:26:45Z

src/cdk/layout/breakpoints-observer.ts

+  /**  A map of all media queries currently being listened for. */
+  private _queries: Map<string, Query> = new Map();
+  /** A subject for all other observables to takeUntil based on. */
+  private _activeSubject: Subject<{}> = new Subject();


Maybe _teardownSubject or _destroySubject?

jelbourn · 2017-09-06T21:28:32Z

src/cdk/layout/media-matcher.ts

+      /**
+       * matchMedia is bound to the window scope intentionally as it is an illegal invocation to
+       * call it from a different scope.
+       */


Use //-style comments

rafaelss95 · 2017-09-07T17:26:34Z

src/demo-app/demo-app/demo-app.ts

@@ -67,6 +67,7 @@ export class DemoApp {
    {name: 'Progress Spinner', route: '/progress-spinner'},
    {name: 'Radio', route: '/radio'},
    {name: 'Ripple', route: '/ripple'},
+    {name: 'Screen Type', route: 'screen-type'},


Missing / -> /screen-type.

rafaelss95 · 2017-09-07T17:28:38Z

src/cdk/layout/media-matcher.ts

+function createEmptyStyleRule(query: string) {
+  if (!styleElementForWebkitCompatibility.has(query)) {
+    try {
+      let style: HTMLStyleElement = document.createElement('style');


HTMLStyleElement is already inferred from .createElement(...).

josephperrott · 2017-09-13T21:33:34Z

@jelbourn

PR is ready for another look.

jelbourn

LGTM, a few last comments

Add the "merge-ready" label when ready

jelbourn · 2017-09-14T00:42:29Z

src/cdk/layout/breakpoints-observer.md

+export class MyWidget {
+  isHandset: Observable<BreakpointState>;
+
+  constructor(bm: BreakpointManager) {


Still has BreakpointManager here

jelbourn · 2017-09-14T00:51:31Z

src/cdk/layout/breakpoints-observer.ts

+      }))
+      .call(takeUntil, this._destroySubject)
+      .call(startWith, mql)
+      .call(map, (nextMql: MediaQueryList) => { return {query: query, matches: nextMql.matches}; })


Don't need the braces and the return, just parens around the object literal

.call(map, (nextMql: MediaQueryList) => ({query: query, matches: nextMql.matches}))

Also, is the explicit MediaQueryList type necessary here? I think it should be inferred.

We need to declare the MediaQueryList type, because the fromEventPattern observable emits an any type unless you use the selector method to transform what is returned, but we would just have to define the type in this method for typescript to know its type. So I just went with declaring the type in the map parameter.

jelbourn · 2017-09-14T00:52:16Z

src/cdk/layout/media-matcher.ts

+ */
+const styleElementForWebkitCompatibility: Map<string, HTMLStyleElement> = new Map();
+
+@Injectable()


Add JsDoc description

jelbourn · 2017-09-26T23:43:48Z

@josephperrott can you rebase? I want to get this in the next sync

josephperrott · 2017-09-27T00:38:48Z

@jelbourn rebased.

jelbourn · 2017-09-28T22:53:34Z

@josephperrott can you rebase one more time?

…dset

josephperrott · 2017-09-29T00:28:08Z

@jelbourn rebased

angular-automatic-lock-bot · 2019-09-07T07:02:30Z

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

josephperrott requested a review from jelbourn August 31, 2017 21:01

josephperrott requested a review from devversion as a code owner August 31, 2017 21:01

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 31, 2017

josephperrott force-pushed the mediaquery branch 8 times, most recently from 9029b8f to 21995fe Compare September 1, 2017 20:21

jelbourn reviewed Sep 5, 2017

View reviewed changes

josephperrott force-pushed the mediaquery branch 2 times, most recently from eda3c6c to 29ceb71 Compare September 6, 2017 21:01

jelbourn reviewed Sep 6, 2017

View reviewed changes

josephperrott force-pushed the mediaquery branch from 29ceb71 to 3d4076d Compare September 7, 2017 14:53

rafaelss95 reviewed Sep 7, 2017

View reviewed changes

josephperrott force-pushed the mediaquery branch from 3d4076d to c82518c Compare September 7, 2017 22:09

jelbourn approved these changes Sep 14, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Sep 14, 2017

josephperrott force-pushed the mediaquery branch 3 times, most recently from 58c34c6 to d375fc6 Compare September 14, 2017 21:08

josephperrott added the action: merge The PR is ready for merge by the caretaker label Sep 14, 2017

josephperrott force-pushed the mediaquery branch 2 times, most recently from cd71113 to 7cc37a3 Compare September 15, 2017 19:25

josephperrott added the pr: merge safe label Sep 19, 2017

willshowell mentioned this pull request Sep 19, 2017

md-toolbar reduces height 1px early #7181

Closed

josephperrott mentioned this pull request Sep 25, 2017

fix(snackbar): swap enter and exit animation curves #6791

Merged

jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 26, 2017

josephperrott force-pushed the mediaquery branch from 7cc37a3 to 490c116 Compare September 27, 2017 00:34

jelbourn added the pr: needs rebase label Sep 28, 2017

Create screen type service to determine screen type: Web, Tablet, Han…

7c0b0df

…dset

josephperrott force-pushed the mediaquery branch from 490c116 to 7c0b0df Compare September 29, 2017 00:27

andrewseguin merged commit 811e504 into angular:master Sep 29, 2017

This was referenced Oct 2, 2017

Design service for helping build responsive components #45

Closed

Shift media query breakpoints 1px #5960

Closed

Responsive drawer #6535

Closed

odahcam mentioned this pull request Dec 27, 2017

feat(grid-list): Make responsive grid list #6777

Closed

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

josephperrott deleted the mediaquery branch March 20, 2020 22:15

		@@ -0,0 +1,26 @@
		### BreakpointsModule

		When including the CDK's `BreakpointsModule`, components can inject `BreakpointsManager` to request


		export type BreakpointQuery = string \| string[];

		export interface BreakpointState {

feat(breakpoints): Create breakpoint management system #6772

feat(breakpoints): Create breakpoint management system #6772

Uh oh!

Conversation

josephperrott commented Aug 31, 2017

Uh oh!

fxck commented Sep 2, 2017

Uh oh!

jelbourn commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephperrott commented Sep 13, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Sep 26, 2017

Uh oh!

josephperrott commented Sep 27, 2017

Uh oh!

jelbourn commented Sep 28, 2017

Uh oh!

josephperrott commented Sep 29, 2017

Uh oh!

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

Uh oh!

Uh oh!