feat: header functionality for the list view #806
Conversation
itssharmasandeep
changed the title
Codecov Report
@@ Coverage Diff @@
## main #806 +/- ##
=======================================
Coverage 85.42% 85.43%
=======================================
Files 792 792
Lines 16197 16198 +1
Branches 2070 2070
=======================================
+ Hits 13837 13838 +1
Misses 2329 2329
Partials 31 31
Continue to review full report at Codecov.
|
| @@ -7,6 +7,33 @@ | |||
| display: flex; | |||
| flex-direction: column; | |||
|
|
|||
| .header { | |||
| flex: 0 0 auto; | |||
There was a problem hiding this comment.
flex: 0 0 auto; you probably don't need this if you are setting a fixed width/height
There was a problem hiding this comment.
Okay, I will check for it.
This comment has been minimized.
This comment has been minimized.
|
|
||
| .header-key { | ||
| flex: 1 1 auto; | ||
| font-weight: 600; |
There was a problem hiding this comment.
Is there a mixin that we can use here?
There was a problem hiding this comment.
I was looking for it but could not find.
I will check more
There was a problem hiding this comment.
Please have a look
| text-transform: uppercase; | ||
| } | ||
| .header-key:nth-child(1) { | ||
| width: 40%; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I can refactor this whole css file using css grid.
There was a problem hiding this comment.
I have made the change, please have a look!!
| @@ -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"> | |||
There was a problem hiding this comment.
nit: class="header-row"
| @@ -18,6 +23,9 @@ import { ChangeDetectionStrategy, Component, Input } from '@angular/core'; | |||
| ` | |||
| }) | |||
| export class ListViewComponent { | |||
| @Input() | |||
| public headerKeys?: [string, string]; | |||
There was a problem hiding this comment.
ListViewRecord also has key and value properties. So, instead of headerKeys should we have two properties: keyHeaderLabel: string and 'valueHeaderLabel: string`
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, like 'headerLabel?: ListHeaderLabel;'
Interface ListHeaderLabel { key: string; value: string;}
There was a problem hiding this comment.
I guess, interface ListHeader { keyLabel: string; valueLabel: string} this would be better?
There was a problem hiding this comment.
Yeah. This is fine too!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| .list-view { | ||
| height: 100%; | ||
| width: 100%; | ||
| overflow: auto; | ||
| display: flex; | ||
| flex-direction: column; | ||
|
|
||
| .header-row { | ||
| @include grid-view(); |
There was a problem hiding this comment.
Nice work isolating column width as variables.
I think you can just add these grid properties to list-view class. No need to add it in both header and data row with flex column in list view.
There was a problem hiding this comment.
Oh I was thinking more like this
.list-view{
height: 100%;
width: 100%;
overflow: auto;
width: 100%;
padding: 8px 0;
display: grid;
grid-template-columns: $key-width $value-width;
align-content: center;
.header-key-label{}
.header-value-label{}
.data-key{}
.data-value{}
}
Is this something we can do?
There was a problem hiding this comment.
But then we'll have to put some styles twice like
- Header shadow and background
- Alternate background for data row and column etc.
There was a problem hiding this comment.
if needed for this, we can take this as follow up work.
I am merging changes for now.
This comment has been minimized.
This comment has been minimized.
| width: 100%; | ||
| word-break: break-word; | ||
| padding: 8px 0; | ||
| & > * { |
There was a problem hiding this comment.
list-view could have a huge number of children. In such cases, this does us more harm than good. We should use a class name here instead of the wildcard.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Description
Header functionality for the list view
Testing
Local testing done
Checklist: