Skip to content

feat: add spacing between title and table when title is set #718

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 24, 2021
Merged

feat: add spacing between title and table when title is set #718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27c5a6b
feat: add spacing between title and table when title is set
jake-bassett Mar 24, 2021
65769af
fix: removed margin prop from table header
jake-bassett Mar 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions projects/dashboards/src/widgets/header/widget-header.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,4 @@ export class WidgetHeaderModel {
} as ModelModelPropertyTypeInstance
})
public link?: LinkWidgetModel;

@ModelProperty({
key: 'top-margin',
type: BOOLEAN_PROPERTY.type
})
public topMargin?: boolean = false;
}
8 changes: 0 additions & 8 deletions projects/distributed-tracing/src/shared/dashboard/widgets/table/table-widget-base.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,6 @@ import { TableWidgetControlCheckboxOptionModel } from './table-widget-control-ch
import { TableWidgetControlSelectOptionModel } from './table-widget-control-select-option.model';

export abstract class TableWidgetBaseModel extends BaseModel {
@ModelProperty({
key: 'title',
displayName: 'Title',
type: STRING_PROPERTY.type
})
// @deprecated
public title?: string;

@ModelProperty({
// tslint:disable-next-line: no-object-literal-type-assertion
type: {
Expand Down
4 changes: 4 additions & 0 deletions ...stributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

.table-content-container {
@include flex-layout(column);

&.titled {
padding-top: 12px;
}
}

.table {
Expand Down
4 changes: 2 additions & 2 deletions ...distributed-tracing/src/shared/dashboard/widgets/table/table-widget-renderer.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,12 @@ import { TableWidgetModel } from './table-widget.model';
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<ht-titled-content
class="table-title"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there's already a [ngClass]="{ 'header-margin': this.model.header?.topMargin }" - is that the same thing? If the two don't have different purposes, can we resolve to one way of doing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header margin is for the table controls header. The new one is for the title that sits above that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a bug though - the header model where this top margin is defined is used for the title, not table controls. I'm guessing when table controls were added, it created an unintentional separation between the title and where that margin was applied. If you look where this is used today, it's only in one spot - backend overview, on a table that doesn't have controls (which is likely why the bug was never caught - it was still rendering the same).

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the places (HT home dash) where the header is used without topMargin being set just look bad - they should have this property set. If we want to remove that property entirely and automatically add the appropriate padding if title is set, I think that works.

image

We just want to make sure we're not double padding it if that property is set, so we should do one or the other.

[title]="this.model.header?.title | htDisplayTitle"
[link]="this.model.header?.link?.url"
[linkLabel]="this.model.header?.link?.displayText"
>
<div class="table-content-container">
<div class="table-content-container" [class.titled]="this.model.header?.title !== undefined">
<ht-table-controls
class="table-controls"
[searchEnabled]="!!this.api.model.getSearchAttribute()"
Expand All @@ -68,7 +69,6 @@ import { TableWidgetModel } from './table-widget.model';

<ht-table
class="table"
[ngClass]="{ 'header-margin': this.model.header?.topMargin }"
[columnConfigs]="this.columnConfigs$ | async"
[metadata]="this.metadata$ | async"
[mode]="this.model.mode"
Expand Down
3 changes: 1 addition & 2 deletions projects/observability/src/pages/apis/backend-detail/overview/backend-overview.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,8 +452,7 @@ export class BackendOverviewComponent {
style: TableStyle.Embedded,
header: {
type: 'widget-header',
title: 'Services',
'top-margin': true
title: 'Services'
},
pageable: false,
columns: [
Expand Down