Skip to content

feat: header functionality for the list view #806

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 8 commits into from
May 4, 2021
Merged

feat: header functionality for the list view #806

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c5148b0
feat: header functionality in the list view
itssharmasandeep Apr 30, 2021
9c9b6b0
Merge remote-tracking branch origin/main into list-view-header
itssharmasandeep Apr 30, 2021
f1403f0
fix: addressing review comments
itssharmasandeep Apr 30, 2021
b02a5a7
Merge branch 'main' into list-view-header
itssharmasandeep May 3, 2021
d547d83
fix: addressing review comments
itssharmasandeep May 3, 2021
6e799a6
Merge branch 'list-view-header' of github.com:hypertrace/hypertrace-u…
itssharmasandeep May 3, 2021
cd27f4f
fix: addressing review comment
itssharmasandeep May 4, 2021
add59cf
Merge branch 'main' into list-view-header
itssharmasandeep May 4, 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
27 changes: 27 additions & 0 deletions projects/components/src/list-view/list-view.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,33 @@
display: flex;
flex-direction: column;

.header {
flex: 0 0 auto;
Copy link
Contributor

@anandtiwary anandtiwary Apr 30, 2021

Choose a reason for hiding this comment

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

flex: 0 0 auto; you probably don't need this if you are setting a fixed width/height

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

Okay, I will check for it.

height: 32px;
width: 100%;
padding: 8px 0;
background: $gray-1;
box-shadow: inset 0px -1px 0px $gray-2;
border-radius: 6px 6px 0px 0px;
display: flex;
align-items: center;

.header-key {
flex: 1 1 auto;
font-weight: 600;
Copy link
Contributor

@anandtiwary anandtiwary Apr 30, 2021

Choose a reason for hiding this comment

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

Is there a mixin that we can use here?

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

I was looking for it but could not find.
I will check more

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

Please have a look

font-size: 12px;
line-height: 14px;
margin-left: 12px;
text-transform: uppercase;
}
.header-key:nth-child(1) {
width: 40%;
Copy link
Contributor

@anandtiwary anandtiwary Apr 30, 2021

Choose a reason for hiding this comment

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

This should be in sync with the data row. Why not use css grids? we can define a fixed column widths that both header and data row can share

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

I can refactor this whole css file using css grid.

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

I have made the change, please have a look!!

}
.header-key:nth-child(2) {
width: 60%;
}
}

.data-row {
flex: 0 0 auto;
min-height: 40px;
Expand Down
15 changes: 13 additions & 2 deletions projects/components/src/list-view/list-view.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { ListViewComponent, ListViewRecord } from './list-view.component';
describe('List View Component', () => {
@Component({
selector: 'ht-test-host-component',
template: ` <ht-list-view [records]="this.records"></ht-list-view> `
template: ` <ht-list-view [records]="this.records" [headerKeys]="this.headerKeys"></ht-list-view> `
})
class TestHostComponent {
public records: ListViewRecord[] = [
Expand All @@ -19,6 +19,8 @@ describe('List View Component', () => {
value: 'Response 2'
}
];

public headerKeys: [string, string] = ['key1', 'key2'];
}

let fixture: ComponentFixture<TestHostComponent>;
Expand All @@ -34,11 +36,20 @@ describe('List View Component', () => {
hostComp = fixture.componentInstance;
});

test('should display rows for each data', () => {
test('should display rows for each data and should display header', () => {
fixture.detectChanges();

const element: HTMLElement = fixture.nativeElement;

const headerElement = element.querySelector<HTMLElement>('.header');
expect(headerElement).not.toBeNull();

const headerKeyElements = element.querySelectorAll<HTMLElement>('.header-key');
expect(headerKeyElements).not.toBeNull();
expect(headerKeyElements.length).toBe(2);
expect(headerKeyElements[0].textContent).toEqual(hostComp.headerKeys[0]);
expect(headerKeyElements[1].textContent).toEqual(hostComp.headerKeys[1]);

const rowElements = element.querySelectorAll<HTMLElement>('.data-row');
expect(rowElements).not.toBeNull();
expect(rowElements.length).toBe(2);
Expand Down
8 changes: 8 additions & 0 deletions projects/components/src/list-view/list-view.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ import { ChangeDetectionStrategy, Component, Input } from '@angular/core';
changeDetection: ChangeDetectionStrategy.OnPush,
template: `
<div class="list-view">
<div *ngIf="this.headerKeys && this.headerKeys.length === 2" class="header">
Copy link
Contributor

@anandtiwary anandtiwary Apr 30, 2021

Choose a reason for hiding this comment

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

nit: class="header-row"

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

Fixed!!

<div class="header-key" *ngFor="let headerKey of this.headerKeys">
<span>{{ this.headerKey }}</span>
</div>
</div>
<div class="data-row" *ngFor="let record of this.records">
<div class="key">
<span>{{ record.key }}</span>
Expand All @@ -18,6 +23,9 @@ import { ChangeDetectionStrategy, Component, Input } from '@angular/core';
`
})
export class ListViewComponent {
@Input()
public headerKeys?: [string, string];
Copy link
Contributor

@anandtiwary anandtiwary Apr 30, 2021

Choose a reason for hiding this comment

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

ListViewRecord also has key and value properties. So, instead of headerKeys should we have two properties: keyHeaderLabel: string and 'valueHeaderLabel: string`

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

Yeah we can have that, But instead of making these two different inputs, we can just create and interface with these two attributes and use that.

Copy link
Contributor

@anandtiwary anandtiwary Apr 30, 2021

Choose a reason for hiding this comment

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

Yeah, like 'headerLabel?: ListHeaderLabel;'

Interface ListHeaderLabel { key: string; value: string;}

Copy link
Contributor Author

@itssharmasandeep itssharmasandeep Apr 30, 2021

Choose a reason for hiding this comment

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

I guess, interface ListHeader { keyLabel: string; valueLabel: string} this would be better?

Copy link
Contributor

@anandtiwary anandtiwary May 1, 2021

Choose a reason for hiding this comment

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

Yeah. This is fine too!


@Input()
public records?: ListViewRecord[];
}
Expand Down