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

fix(portal): detect changes for portal hostview while before attaching. #4370

Merged
merged 1 commit into from
Jun 7, 2017
Merged

fix(portal): detect changes for portal hostview while before attaching. #4370

merged 1 commit into from
Jun 7, 2017

Conversation

josephperrott
Copy link
Member

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 3, 2017
@josephperrott josephperrott force-pushed the portal branch 4 times, most recently from 4d186ff to 6f6cc27 Compare May 9, 2017 01:47
@josephperrott josephperrott force-pushed the portal branch 2 times, most recently from f085a7f to 76aa323 Compare May 10, 2017 19:13
@josephperrott
Copy link
Member Author

@jelbourn ready for you to take a look

jelbourn
jelbourn reviewed May 11, 2017
View reviewed changes
src/lib/core/portal/dom-portal-host.ts
@@ -29,21 +29,17 @@ export class DomPortalHost extends BasePortalHost {
*/
attachComponentPortal<T>(portal: ComponentPortal<T>): ComponentRef<T> {
let componentFactory = this._componentFactoryResolver.resolveComponentFactory(portal.component);
let componentRef: ComponentRef<T>;
let componentRef = componentFactory.create(portal.injector || this._defaultInjector);
componentRef.hostView.detectChanges();
Copy link
Member

Choose a reason for hiding this comment

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

Are you able to create a unit test that enforces this behavior?

@josephperrott josephperrott force-pushed the portal branch 5 times, most recently from f7b6f53 to bdb9e7f Compare May 18, 2017 01:38
@josephperrott
Copy link
Member Author

Should be ready for another look.

I updated MdDatepickerContent to use the safe navigation operator. This is necessary as the newly created ComponentPortal<MdDatepicker> instance is immediately change detected. We need to guard against undefined and null since the inputs aren't guaranteed to be populated.

cc:@mmalerba as this is modifying the newly created MdDatepicker

jelbourn
jelbourn approved these changes May 18, 2017
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM on the portal stuff

@mmalerba should take a look for the datepicker

@jelbourn jelbourn requested a review from mmalerba May 18, 2017 22:58
@mmalerba
Copy link
Contributor

LGTM for datepicker

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jun 2, 2017
@andrewseguin andrewseguin merged commit 28d2ddd into angular:master Jun 7, 2017
@willshowell willshowell mentioned this pull request Jun 11, 2017
Closed
@willshowell
Copy link
Contributor

@jelbourn @mmalerba @josephperrott the CD changes made here are causing several regressions with the datepicker in popup view

  1. Unable to start in year view
  2. Start at date has no effect
  3. "Prev month" button initially enabled when min date in current month
  4. When setting min/max dates within current month, all days are enabled until month view is refreshed
  5. Active date is not highlighted until month view is refreshed

ref #5065

jelbourn added a commit to jelbourn/components that referenced this pull request Jun 14, 2017
@jelbourn
Revert "fix(portal): detect changes for portal hostview while before …
bdae634 
…attaching. (angular#4370)"

This reverts commit 28d2ddd.
@josephperrott josephperrott mentioned this pull request Jun 14, 2017
Merged
kara pushed a commit that referenced this pull request Jun 15, 2017
@jelbourn @kara
Revert "fix(portal): detect changes for portal hostview while before …
99fc668 
…attaching. (#4370)" (#5131)
@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 6, 2019
@josephperrott josephperrott deleted the portal branch March 20, 2020 22:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

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.
6 participants
@josephperrott @mmalerba @willshowell @jelbourn @googlebot @andrewseguin