feat: remove the need for forRoot on material NgModules #2556

Merged
merged 2 commits into from Jan 11, 2017

Projects

None yet

7 participants

@jelbourn
Member
jelbourn commented Jan 6, 2017
  • Dialog and snackbar use their parent's state if there is one
  • Other service that we want to be singletons and have state use a useFactory provider to get the ancestor and provide that if it exists

cc @IgorMinar can you provide thoughts on this?

cc FYI @josephperrott @mmalerba @andrewseguin @tinayuangao

@jelbourn jelbourn requested a review from kara Jan 6, 2017
@googlebot googlebot added the cla: yes label Jan 6, 2017
@jelbourn
Member
jelbourn commented Jan 6, 2017

Seeing some failures w/ AoT that don't quite make sense to me. Consulting w/ core team.

@kara

Typos and a question

@@ -88,3 +88,11 @@ export class ScrollDispatcher {
}
}
+export const SCROLL_DISPATCHER_PROVIDER = {
+ // If there is already an ScrollDispatcher available, use that. Otherwise, provide a new one.
@kara
kara Jan 6, 2017 Collaborator

Nit: an ScrollDispatcher -> a ScrollDispatcher

@@ -56,3 +55,12 @@ export class ViewportRuler {
return {top, left};
}
}
+
+export const VIEWPORT_RULER_PROVIDER = {
+ // If there is already an ViewportRuler available, use that. Otherwise, provide a new one.
@kara
kara Jan 6, 2017 Collaborator

Nit: an ViewportRuler -> a ViewportRuler

src/lib/core/a11y/live-announcer.ts
@@ -62,3 +63,15 @@ export class LiveAnnouncer {
}
}
+
+export const LIVE_ANNOUNCER_PROVIDER = {
+ // If there is already an LiveAnnouncer available, use that. Otherwise, provide a new one.
@kara
kara Jan 6, 2017 Collaborator

Nit: an LiveAnnouncer -> a LiveAnnouncer

@@ -167,10 +166,11 @@ export class MdAnchor extends MdButton {
declarations: [MdButton, MdAnchor],
@kara
kara Jan 6, 2017 Collaborator

Do you need to provide VIEWPORT_RULER_PROVIDER up here since you removed below? Same with checkbox.

@jelbourn
jelbourn Jan 7, 2017 Member

Nope, it is not included by way of MdRippleModule

@kara
kara Jan 7, 2017 Collaborator

Cool, just double-checking

@kara
kara approved these changes Jan 7, 2017 View changes

LGTM

@kara kara added pr: lgtm and removed pr: needs review labels Jan 7, 2017
@jelbourn jelbourn feat: remove the need for forRoot on material NgModules
147a077
@jelbourn
Member
jelbourn commented Jan 7, 2017

Also cc FYI @DevVersion @crisbeto @EladBezalel

@IgorMinar
Member

@jelbourn looks reasonable. please file an issue @ angular/angular if you haven't yet. Let's see if the core can help with this pattern as it looks better to me from the user perspective than the forRoot pattern.

@DevVersion
Member

@jelbourn Were the errors on the AOT mode valid? - Also nice change. Should avoid future provider issues as in #2538

@crisbeto

LGTM, this should close #1150 as well.

@jelbourn
Member
jelbourn commented Jan 9, 2017

@DevVersion yes, it caught some real errors

@tinayuangao tinayuangao Merge branch 'master' into cmop-fact-resolver
0f59fec
@tinayuangao tinayuangao merged commit b49bfce into angular:master Jan 11, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment