Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jan 11, 2017
Merged

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

merged 2 commits into from
Jan 11, 2017

Conversation

jelbourn
Copy link
Member

@jelbourn 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 January 6, 2017 20:12
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 6, 2017
@jelbourn
Copy link
Member Author

jelbourn commented Jan 6, 2017

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

kara
kara reviewed Jan 6, 2017
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typos and a question

src/lib/core/overlay/scroll/scroll-dispatcher.ts
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: an ScrollDispatcher -> a ScrollDispatcher

src/lib/core/overlay/position/viewport-ruler.ts
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: an LiveAnnouncer -> a LiveAnnouncer

src/lib/button/button.ts
@@ -167,10 +166,11 @@ export class MdAnchor extends MdButton {
declarations: [MdButton, MdAnchor],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it is not included by way of MdRippleModule

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, just double-checking

kara
kara approved these changes Jan 7, 2017
View reviewed changes
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn force-pushed the cmop-fact-resolver branch from e8dc761 to 147a077 Compare January 7, 2017 00:46
@jelbourn
Copy link
Member Author

jelbourn commented Jan 7, 2017

Also cc FYI @devversion @crisbeto @EladBezalel

@IgorMinar
Copy link
Contributor

@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
Copy link
Member

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

crisbeto
crisbeto approved these changes Jan 9, 2017
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this should close #1150 as well.

@jelbourn
Copy link
Member Author

jelbourn commented Jan 9, 2017

@devversion yes, it caught some real errors

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jan 10, 2017
@tinayuangao tinayuangao merged commit b49bfce into angular:master Jan 11, 2017
@devversion devversion mentioned this pull request Jan 11, 2017
Merged
@jelbourn jelbourn mentioned this pull request Jan 12, 2017
Closed
This was referenced Jan 24, 2017
Closed
Closed
@adamk33n3r adamk33n3r mentioned this pull request Feb 7, 2017
Closed
@adriancarriger adriancarriger mentioned this pull request Jun 6, 2017
Open
@jelbourn jelbourn deleted the cmop-fact-resolver branch September 13, 2017 04:37
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kara kara kara approved these changes

@crisbeto crisbeto crisbeto approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jelbourn @IgorMinar @devversion @kara @crisbeto @googlebot @tinayuangao