Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
@import 'font';
@import 'color-palette';

.string-array-cell {
display: flex;
flex-direction: row;
align-items: center;

.first-item {
padding-right: 8px;
}

.summary-text {
padding: 0 6px;
background-color: $gray-2;
border-radius: 4px;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { TableCellNoOpParser } from '@hypertrace/components';
import { createComponentFactory } from '@ngneat/spectator/jest';
import { tableCellDataProvider, tableCellProviders } from '../../test/cell-providers';
import { StringArrayTableCellRendererComponent } from './string-array-table-cell-renderer.component';

describe('String array table cell renderer component', () => {
const buildComponent = createComponentFactory({
component: StringArrayTableCellRendererComponent,
providers: [
tableCellProviders(
{
id: 'test'
},
new TableCellNoOpParser(undefined!)
)
],

shallow: true
});

test('should render an array with one item as expected', () => {
const spectator = buildComponent({
providers: [tableCellDataProvider(['first-item'])]
});

expect(spectator.query('.first-item')).toHaveText('first-item');
expect(spectator.query('.summary-text')).toHaveText('');
});

test('should render an empty array as expected', () => {
const spectator = buildComponent({
providers: [tableCellDataProvider([])]
});

expect(spectator.query('.first-item')).toHaveText('-');
expect(spectator.query('.summary-text')).toHaveText('');
});

test('should render array with multiple items as expected', () => {
const spectator = buildComponent({
providers: [tableCellDataProvider(['first-item', 'second-item', 'third-item'])]
});

expect(spectator.query('.first-item')).toHaveText('first-item');
expect(spectator.query('.summary-text')).toHaveText('+2');
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { ChangeDetectionStrategy, Component, Inject, OnInit, TemplateRef, ViewChild } from '@angular/core';
import { TableColumnConfig, TableRow } from '../../../table-api';
import {
TABLE_CELL_DATA,
TABLE_COLUMN_CONFIG,
TABLE_COLUMN_INDEX,
TABLE_DATA_PARSER,
TABLE_ROW_DATA
} from '../../table-cell-injection';
import { TableCellParserBase } from '../../table-cell-parser-base';
import { TableCellRenderer } from '../../table-cell-renderer';
import { TableCellRendererBase } from '../../table-cell-renderer-base';
import { CoreTableCellParserType } from '../../types/core-table-cell-parser-type';
import { CoreTableCellRendererType } from '../../types/core-table-cell-renderer-type';
import { TableCellAlignmentType } from '../../types/table-cell-alignment-type';

@Component({
selector: 'ht-string-array-table-cell-renderer',
styleUrls: ['./string-array-table-cell-renderer.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div class="string-array-cell">
<span class="first-item">{{ this.firstItem }}</span>
<span class="summary-text" [htTooltip]="this.summaryTooltip">{{ this.summaryText }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think the tooltip should be across the whole cell, and show all values. This actually just came up when @anandtiwary implemented basically exactly this component... can we share an underlying component? let me go find it

Copy link
Contributor

Choose a reason for hiding this comment

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

Linked in the summary comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaron-steinfeld the link you used is for this PR. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so? Linked to #615, this is #655 no matter how similar they might look :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. My browser was not redirecting when I clicked on the link. So I assumed its this page. I'll check that PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaron-steinfeld Checked that PR. I'm not sure if we should use that here. #655 adds them as summary values which has a certain format. If there are no icons present, it shows a dot. To circumvent that usecase we'll have to modify the summary value component's behaviour. Also override fonts etc. I don't think we should do that just to reuse it. Our usecase here is plain string array display.

We cannot take this out into a component and have it used there either because that component is built for summary values usecase. The API is more complex with optional icons, labels etc.

One thing we can do is to write a wrapper which covers both cases that'd use summary value / plain text based on requirement. But then we'd be writing a component with a pretty confusing/complicated API to share one template and couple lines of code.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK - definitely not worth maintaining that level of difference in one component. I just wish we'd stop using similar but slightly unique components everywhere...

Copy link
Contributor

Choose a reason for hiding this comment

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

We should really consolidate the two styles. I think per michael it is the same component. This would lead to inconsistency, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we pull this one out into a standalone, simple component that we use in the renderer? We can then leave it as tech debt to make the summaries component use that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll file a tech debt for this.


<ng-template #summaryTooltipElement>
<div *ngFor="let value of this.value.slice(1, this.value.length)">{{ value }}</div>
</ng-template>
</div>
`
})
@TableCellRenderer({
type: CoreTableCellRendererType.StringArray,
alignment: TableCellAlignmentType.Left,
parser: CoreTableCellParserType.NoOp
})
export class StringArrayTableCellRendererComponent extends TableCellRendererBase<string[]> implements OnInit {
public firstItem!: string;
public summaryText!: string;

@ViewChild('summaryTooltipElement')
public summaryTooltip!: TemplateRef<unknown>;

public constructor(
@Inject(TABLE_COLUMN_CONFIG) columnConfig: TableColumnConfig,
@Inject(TABLE_COLUMN_INDEX) index: number,
@Inject(TABLE_DATA_PARSER) parser: TableCellParserBase<string[], string[], unknown>,
@Inject(TABLE_CELL_DATA) cellData: string[],
@Inject(TABLE_ROW_DATA) rowData: TableRow
) {
super(columnConfig, index, parser, cellData, rowData);
}

public ngOnInit(): void {
super.ngOnInit();

this.firstItem = this.value[0] ?? '-';
this.summaryText = this.value.length > 1 ? `+${this.value.length - 1}` : '';
}
}
7 changes: 5 additions & 2 deletions projects/components/src/table/cells/table-cells.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { TableCellTimestampParser } from './data-parsers/table-cell-timestamp-pa
import { CodeTableCellRendererComponent } from './data-renderers/code/code-table-cell-renderer.component';
import { IconTableCellRendererComponent } from './data-renderers/icon/icon-table-cell-renderer.component';
import { NumericTableCellRendererComponent } from './data-renderers/numeric/numeric-table-cell-renderer.component';
import { StringArrayTableCellRendererComponent } from './data-renderers/string-array/string-array-table-cell-renderer.component';
import { TableDataCellRendererComponent } from './data-renderers/table-data-cell-renderer.component';
import { TextTableCellRendererComponent } from './data-renderers/text/text-table-cell-renderer.component';
import { TimeAgoTableCellRendererComponent } from './data-renderers/time-ago/time-ago-table-cell-renderer.component';
Expand Down Expand Up @@ -64,7 +65,8 @@ export const TABLE_CELL_PARSERS = new InjectionToken<unknown[][]>('TABLE_CELL_PA
TextTableCellRendererComponent,
TimestampTableCellRendererComponent,
TimeAgoTableCellRendererComponent,
CodeTableCellRendererComponent
CodeTableCellRendererComponent,
StringArrayTableCellRendererComponent
],
providers: [
{
Expand All @@ -77,7 +79,8 @@ export const TABLE_CELL_PARSERS = new InjectionToken<unknown[][]>('TABLE_CELL_PA
TextTableCellRendererComponent,
TimestampTableCellRendererComponent,
TimeAgoTableCellRendererComponent,
CodeTableCellRendererComponent
CodeTableCellRendererComponent,
StringArrayTableCellRendererComponent
],
multi: true
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export const enum CoreTableCellRendererType {
Icon = 'icon',
Number = 'number',
RowExpander = 'row-expander',
StringArray = 'string-array',
Text = 'text',
Timestamp = 'timestamp',
TimeAgo = 'time-ago'
Expand Down