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.

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

Add NgModules for material #950

Merged
merged 23 commits into from
Aug 8, 2016
Merged

Add NgModules for material #950

merged 23 commits into from
Aug 8, 2016

Conversation

jelbourn
Copy link
Member

@jelbourn jelbourn commented Aug 5, 2016

R: @hansl @kara

This PR includes:

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 5, 2016
@@ -12,7 +12,8 @@ export {
export {
Copy link
Contributor

Choose a reason for hiding this comment

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

Export a core module from here, or export all sub-modules through this file.

Copy link
Member Author

@jelbourn jelbourn Aug 6, 2016

Choose a reason for hiding this comment

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

Exported individual modules and added an MdCoreModule

@hansl
Copy link
Contributor

hansl commented Aug 5, 2016

Globally the feel is good. Please address my previous comment.

RtlModule,
];

@NgModule(({

Choose a reason for hiding this comment

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

one too many parenthesis here?

template: `
<md-button-toggle>Yes</md-button-toggle>
`
})
class StandaloneButtonToggle { }

const TEST_COMPONENTS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

These are only used once, please inline them.

@@ -122,7 +124,7 @@ function detectChangesForDialogOpen(fixture: ComponentFixture<ComponentWithChild
// Two rounds of change detection are necessary: one to *create* the dialog container, and
// another to cause the lifecycle events of the container to run and load the dialog content.
fixture.detectChanges();
setTimeout(() => fixture.detectChanges(), 50);
setTimeout(() => fixture.detectChanges(), 150);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit; is this necessary and can we make it async? 150 is really long for a unittest.

Copy link
Member Author

Choose a reason for hiding this comment

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

Leftover from debugging, reverted.
(and will likely go away when the zone fix is in)

@jelbourn
Copy link
Member Author

jelbourn commented Aug 8, 2016

Removed all TEST_COMPONENTS

@jelbourn
Copy link
Member Author

jelbourn commented Aug 8, 2016

@hansl all comments should be resolved

@hansl
Copy link
Contributor

hansl commented Aug 8, 2016

LGTM.

@jelbourn jelbourn merged commit ca351b2 into angular:master Aug 8, 2016
@tb tb mentioned this pull request Aug 26, 2016
@jelbourn jelbourn deleted the ngmodules branch September 13, 2017 04:33
@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.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants