-
Notifications
You must be signed in to change notification settings - Fork 11
fix: adding layout trigger to titled container #352
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
Changes from all commits
41f18ec
cae3003
0709a62
3749ea4
0201081
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,16 @@ | ||
import { AfterViewInit, Directive, EventEmitter, Output } from '@angular/core'; | ||
import { Directive, EventEmitter, Output } from '@angular/core'; | ||
import { LayoutChangeService, SubscriptionLifecycle } from '@hypertrace/common'; | ||
|
||
@Directive({ | ||
selector: '[htLayoutChange]', | ||
providers: [SubscriptionLifecycle, LayoutChangeService] | ||
}) | ||
export class LayoutChangeDirective implements AfterViewInit { | ||
export class LayoutChangeDirective { | ||
@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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not sized correctly. Here is what happens:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we try it in onInit instead of the constructor? That would have the same affect, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lemme try There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't work. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It didn't. It somehow still asking for LayoutService. Need to look more There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a new mockDashboardWidgetProviders. |
||
<ht-label *ngIf="this.shouldShowTitleInHeader" [label]="this.title" class="title"></ht-label> | ||
<ht-link [paramsOrUrl]="this.link" class="link" *ngIf="this.link"> | ||
<ht-button | ||
|
@@ -51,6 +51,10 @@ export class TitledContentComponent { | |
@Input() | ||
public linkLabel?: string; | ||
|
||
public get shouldShowHeader(): boolean { | ||
return this.shouldShowTitleInHeader || this.headerControl !== undefined; | ||
} | ||
|
||
private get shouldShowTitle(): boolean { | ||
return !isEmpty(this.title) && !this.hideTitle; | ||
} | ||
|
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.
Changed this for testing purposes. Was unable to create mock service without it