Skip to content

fix(overlay): wait until after change detection to position overlays #6527

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
Merged
merged 11 commits into from
Oct 10, 2017
Merged

fix(overlay): wait until after change detection to position overlays #6527

merged 11 commits into from
Oct 10, 2017

Conversation

jelbourn
Copy link
Member

No description provided.

@jelbourn jelbourn added the in progress This issue is currently in progress label Aug 17, 2017
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 17, 2017
@jelbourn jelbourn force-pushed the overlay-position-timing branch from 92e1fb3 to ef10c6c Compare September 6, 2017 17:58
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Sep 6, 2017
@angular angular deleted a comment from googlebot Sep 6, 2017
@crisbeto crisbeto force-pushed the overlay-position-timing branch from 0b77b83 to 9555c67 Compare September 11, 2017 18:26
@crisbeto crisbeto added pr: needs review and removed in progress This issue is currently in progress labels Sep 11, 2017
@angular angular deleted a comment from googlebot Sep 11, 2017
@angular angular deleted a comment from googlebot Sep 11, 2017
@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Sep 11, 2017
@jelbourn jelbourn changed the title wip change when overlays are positioned fix(overlay): wait until after change detection to position overlays Sep 11, 2017
kara
kara approved these changes Sep 11, 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, one question

src/cdk/overlay/position/connected-position-strategy.ts Outdated
@@ -147,6 +147,11 @@ export class ConnectedPositionStrategy implements PositionStrategy {
* allows one to re-align the panel without changing the orientation of the panel.
*/
recalculateLastPosition(): void {
// If the overlay had never been positioned before, do nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: had -> has

src/cdk/overlay/overlay-ref.ts
// before attempting to position it, as the position may depend on the size of the rendered
// content.
first.call(this._ngZone.onStable.asObservable()).subscribe(() => {
this.updatePosition();
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment implies waiting for the zone to be stable before positioning, but it looks like we are also updating the position before the zone is stable on line 58. Is it intentional to update the position twice? If so, maybe update the comment to clarify.

Copy link
Member

Choose a reason for hiding this comment

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

The first call shouldn't be necessary. @jelbourn I ended up removing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, not sure why that was still there

@crisbeto crisbeto force-pushed the overlay-position-timing branch from 80b1508 to 0be6a63 Compare September 11, 2017 19:00
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Sep 11, 2017
@angular angular deleted a comment from googlebot Sep 11, 2017
@jelbourn jelbourn added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Sep 12, 2017
@crisbeto crisbeto force-pushed the overlay-position-timing branch from 0be6a63 to 6f85728 Compare September 12, 2017 20:21
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Sep 12, 2017
@angular angular deleted a comment from googlebot Sep 12, 2017
@kara
Copy link
Contributor

kara commented Sep 12, 2017

LGTM

@kara kara added the pr: lgtm label Sep 12, 2017
@crisbeto crisbeto force-pushed the overlay-position-timing branch from 69cda2a to 96683fc Compare October 3, 2017 17:14
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Oct 3, 2017
@crisbeto crisbeto added cla: yes PR author has agreed to Google's Contributor License Agreement action: merge The PR is ready for merge by the caretaker and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Oct 3, 2017
@crisbeto
Copy link
Member

crisbeto commented Oct 3, 2017

Should be ready to go now.

@angular angular deleted a comment from googlebot Oct 3, 2017
@kara kara added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Oct 3, 2017
@jelbourn
Copy link
Member Author

jelbourn commented Oct 6, 2017

@kara can you describe the presubmit failure here?

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Oct 6, 2017
@andrewseguin
Copy link
Contributor

Needs CLA

@andrewseguin andrewseguin removed the action: merge The PR is ready for merge by the caretaker label Oct 9, 2017
@andrewseguin
Copy link
Contributor

Also, not getting any presubmit errors. @kara do you know what they were?

@jelbourn
Copy link
Member Author

jelbourn commented Oct 9, 2017

@andrewseguin the CLA is fine, it's just me and Kristiyan

@andrewseguin andrewseguin merged commit f299d25 into angular:master Oct 10, 2017
@jelbourn jelbourn deleted the overlay-position-timing branch April 2, 2018 22:30
@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 8, 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 Awaiting requested review from crisbeto

@josephperrott josephperrott Awaiting requested review from josephperrott

Assignees

@crisbeto crisbeto

Labels
cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jelbourn @kara @mmalerba @crisbeto @googlebot @andrewseguin