fix: adding layout trigger to titled container #352
Conversation
- Triggering layout initialization at the time of creation rather than after view init.
| @Output('htLayoutChange') | ||
| public readonly changeEmitter: EventEmitter<void> = new EventEmitter(); | ||
|
|
||
| public constructor(private readonly layoutChange: LayoutChangeService, subscriptionLifecycle: SubscriptionLifecycle) { | ||
| subscriptionLifecycle.add(layoutChange.layout$.subscribe(this.changeEmitter)); | ||
| } | ||
|
|
||
| public ngAfterViewInit(): void { | ||
| this.layoutChange.initialize(); |
There was a problem hiding this comment.
@aaron-steinfeld This is the fix for the layout change issue. Since we initialize/register the bounding element after view init, during this time the bounding element already has the new width/height. When layout trigger emits a new dom change, such events get filtered out as it doesn't see any change in dimension.
There was a problem hiding this comment.
Hmm I think I understand, but if everything is sized correctly after view init, do we even need the layout change? We probably shouldn't do our first measurement in any rendering code until after view init.
There was a problem hiding this comment.
It's not sized correctly. Here is what happens:
- Gauge component gets initialized.
- At this time it gets 200px height
- htlayoutChange directive gets initialized but the host element is not registered.
- On first input change, Gauge component renders the widget with 200px height
- Titled content components view changes and it reduces the height of gauge from 200 to 170px
- htlayoutChange now registers host element with layout service
- Gauge is now registered in layout service with 170px height
- htLayoutChangeTrigger triggers a layout change. This publish gets filtered out when it compares the current height of gauge with its registered height.
- Nothings gets emitted.
There was a problem hiding this comment.
So if we move the first render in gauge afterViewInit would that fix the issue? And would it make sense? I'm thinking yes since it's sizing sensitive, it also prevents the double render.
There was a problem hiding this comment.
I have seen a delay when i tried this last week. But shouldn't we fix this from the layout directive itself as it is likely to affect other places as well?
There was a problem hiding this comment.
Can we try it in onInit instead of the constructor? That would have the same affect, right?
| @@ -10,7 +10,7 @@ import { TitledHeaderControlDirective } from './header-controls/titled-header-co | |||
| changeDetection: ChangeDetectionStrategy.OnPush, | |||
| template: ` | |||
| <div class="titled-content-container"> | |||
| <div class="header"> | |||
| <div class="header" [htLayoutChangeTrigger]="this.shouldShowHeader"> | |||
There was a problem hiding this comment.
Using this trigger here fails a lot of test. We now have to provide layout change service almost everywhere. Mocking the directive didn't help. Was wondering if we could avoid it.
There was a problem hiding this comment.
I'd guess mostly dashboards - other tests shouldn't be using the real components. I was going to suggest adding it to mockDashboardProviders` but it looks like that was already done - did that help? Not every dashboard test may be using that mock set.
There was a problem hiding this comment.
It didn't. It somehow still asking for LayoutService. Need to look more
There was a problem hiding this comment.
Added a new mockDashboardWidgetProviders.
- moving dashboard provides to an exported variable - This would make it easier to provide essential services without being too verbose in every test - Needed to provide LayoutChangeService with me current change as we added it in the titled content component - Also, this fixes the navigation$ pipe console warning which usually comes during test runs
| .subscribe(this.layoutChangeSubject); | ||
| } | ||
|
|
||
| public getLayoutChangeEventObservable(): Observable<LayoutChangeEvent> { |
There was a problem hiding this comment.
Changed this for testing purposes. Was unable to create mock service without it
Codecov Report
@@ Coverage Diff @@
## main #352 +/- ##
========================================
Coverage 86.25% 86.25%
========================================
Files 733 733
Lines 14890 14892 +2
Branches 1762 1899 +137
========================================
+ Hits 12843 12845 +2
+ Misses 2016 2014 -2
- Partials 31 33 +2
Continue to review full report at Codecov.
|
than after view init.
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.