fix(dynamic overlay): prevent multiple onStable subscriptions #2494
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Otherwise, every time the `show` method is called, we create an additional subscription. It leads to multiple `updatePosition` calls.
Dynamic overlay handler compares context and configs objects by reference. So when we passing a new object to `context` and `overlayConfig` methods dynamic overlay handler threats them as a new objects and updates these properties on the dynamic overlay. Changes on dynamic overlay context and config causes disposal of existing overlay and creation of a new one. This commit changes ensures config and context objects update only once at least one of it's properties got updated. This prevent unnecessary overlay disposal described above.
yggg
changed the title
As content affects overlay dimensions.
Instead of using `ngOnChanges` as it won't be called if some properties have been changed on component instances directly (not in the template).
Codecov Report
@@ Coverage Diff @@
## master #2494 +/- ##
==========================================
+ Coverage 80.41% 80.48% +0.06%
==========================================
Files 244 244
Lines 7180 7224 +44
Branches 787 792 +5
==========================================
+ Hits 5774 5814 +40
- Misses 1171 1172 +1
- Partials 235 238 +3
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please read and mark the following check list before creating a pull request:
Short description of what this resolves:
The main fix is 419ca7c. After the change, dynamic overlay unsubscribes from
zone.onStableonce overlay got destroyed. Before, every time theshowmethod was called, we were creating an additional subscription, which leads to multipleupdatePositioncalls.Changes from 3379373 is an additional fix, which ensures dynamic overlay context and config properties updating only once some property actually changed. Before, because of we were passing a new object to
overlayConfigandcontextmethods overlay was recreated on everyngOnChangesrun. See commit description for details. UPD: 1763905 is a refactor of the way overlay config objects updated. First I decided to use change object fromOnChangeshook, but we have tests which check if direct manipulation of component instance properties (not in the template) is also picked up.OnChangeshook doesn't track such changes, so I moved overlay config objects update code to the setters. This way you can change properties from anywhere (from template or component instance properties) and it'll work anyway.The third commit 1840a9d, has an additional small fix. We need to call
updatePositiononce overlay content change, as it could affect overlay dimensions. It worked fine before because of the bug fixed in this PR.