-
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
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
5587f44
to
1e73167
Compare
@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 stuff
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.
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! 😄
1e73167
to
5380f92
Compare
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? |
539083b
to
fd1e110
Compare
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 |
c47f6b7
to
009c0a7
Compare
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 😄 |
64a40e7
to
6f7332b
Compare
@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-directive
when_attachOverlay()
is called if its already been created.Replaces #6489