-
Notifications
You must be signed in to change notification settings - Fork 6.8k
fix(overlay): dimension not updated after init #8765
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
Conversation
crisbeto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also rename the PR? It's not really a feature on the select, but rather a bugfix on the overlay. It can be something along the lines of fix(overlay): dimension not updated after init.
src/lib/select/select.spec.ts
Outdated
| it('should set the width of the overlay based on the trigger and resize', async(() => { | ||
| trigger.style.width = '200px'; | ||
| fixture.detectChanges(); | ||
| fixture.whenStable().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we're moving away from async tests, because they tend to be slower and flakier. Can you try to turn this into a fakeAsync one? All you have to do is replace the fixture.whenStable callbacks with a flush call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, will do that
|
@crisbeto Done, renamed the commit and replaced |
src/lib/select/select.spec.ts
Outdated
| it('should set the width of the overlay based on the trigger and resize', fakeAsync(() => { | ||
| trigger.style.width = '200px'; | ||
| fixture.detectChanges(); | ||
| fixture.whenStable().then(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't use fixture.whenStable inside a fakeAsync test. What ends up happening is that the test will complete before the callback is invoked. What I meant before is taking something like this:
fixture.detectChanges();
fixture.whenStable(() => {
// do stuff
});And turning it into this:
fixture.detectChanges();
flush();
// do stuffThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated that already, missed it before haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you have one left over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh snap, my bad... thats what i get for trying to do it fast haha.. fixing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now its done for sure! 😄
crisbeto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Can you check the errors on this? |
|
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 all authors are ok 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 |
|
Trying to fix the commits haha sorry about that. |
|
Ok, now everything is rebased. Hoping that helps with the CI tests. Does the |
|
Hmm.. im still not sure why the other tests are failing.. cant seem to figure out why would my code change affect those tests.. any ideas @andrewseguin @crisbeto ? |
|
Is there any news for this issue? |
|
@emoralesb05 could you rebase this PR? |
|
Sure, gonna try doing it today @jelbourn 😄 |
|
@emoralesb05 are you going to resubmit the PR? Otherwise I don't mind fixing it and it may be easier for me to get it through. |
|
Sorry @crisbeto forgot to push after the rebase. There is a unit test issue i'm seeing with tooltip but it seems unrelated to the commit since it was failing when i ran it in Hopefully, we can merge this PR.. and no hard feelings if its easier to close this one and just fix it yourself @crisbeto haha its been opened for sooooo long that i just want to see that fixed. |
|
I'm not sure why the Bazel check is failing, but it shouldn't be because of the changes in this PR. Do you have an idea @jelbourn? |
|
Looks like a flake |
|
@emoralesb05 there's a process we have to follow. we first run a presubmit against google's codebase and watch for any test failures, once all the tests look good it can be merged |
|
So do we need to trigger the tests again @mmalerba ?? |
|
nope, nothing for you to do, I just need to do the presubmit |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
when changing the width of the mat-select (e.g. using flex box) after its rendered, the template[cdk-connected-overlay] does not update its minWidth and keeps the initial minWidth every time its opened.
This issue happens because the cdk-connected-overlay sets the width, minWidth, etc etc only once and never updates it again.
StackBlitz replication: https://stackblitz.com/edit/angular-material2-issue-jbtqvx
Proposed fix in PR
We just need to update the size of the
overlay-directivewhen_attachOverlay()is called if its already been created.Replaces #6489