feat(stark-ui): initial implementation of Stark Table component (supporting nested data, sorting and filtering) #501

christophercr · 2018-07-12T11:49:04Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NationalBankBelgium/stark/blob/master/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #406

What is the new behavior?

New stark-table component integrated in stark-ui package.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

christophercr · 2018-07-12T11:50:46Z

I've added some interfaces related to the Action bar but those should be removed once the PR about the Action Bar component is merged.

So please don't merge this PR yet, I have to rebase it as soon as the other PR is merged.

christophercr · 2018-07-12T11:59:58Z

I've updated #406 to include the list of features that should be supported.

coveralls · 2018-07-12T13:57:10Z

Coverage decreased (-1.2%) to 93.533% when pulling d2ad745 on christophercr:feature/stark-table into 765e5ef on NationalBankBelgium:master.

christophercr · 2018-07-13T15:08:21Z

I've rebased this PR so the action bar interfaces are removed from the Table folder ;)

SuperITMan · 2018-07-13T15:56:52Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	 * Whether the column is sortable or not. Default: true
+	 */
+	@Input() public sortable: boolean = false;
+	@Input() public sortPriority: number;


Could you please add some documentation for the bindings ? 😊

SuperITMan · 2018-07-16T06:14:57Z

packages/stark-ui/src/modules/table/components/column.component.html

+	<th mat-header-cell *matHeaderCellDef [ngClass]="{'sortable': sortable, 'filtering': filterValue}">
+		<div class="header-cell-content">
+			<div [ngSwitch]="sortDirection" class="sort-header" (click)="onSortChange()">
+				<span>{{ headerLabel || name }}</span>


I think this should be translated as it was the case in the old Stark.

Not for now, translation is not part of this PR, as I mentioned in the description, this is only about nested data, sorting and filtering, otherwise this PR will be huge.

Translation and other features will be part of another PR 😉

Ok. That means we'll have to be really careful to not forget anything in the next PR which will contain translations

SuperITMan · 2018-07-16T06:26:49Z

packages/stark-ui/src/modules/table/components/column.component.html

+						   (keyup)="onFilterChange()">
+				</mat-form-field>
+				<button mat-icon-button (click)="onClearFilter()">
+					<mat-icon svgIcon="close"></mat-icon>


Could you please add a tooltip here ?

SuperITMan · 2018-07-16T06:27:20Z

packages/stark-ui/src/modules/table/components/column.component.html

+				<span *ngIf="sortPriority < 1000 && sortDirection" class="priority">{{ sortPriority }}</span>
+			</div>
+			<button *ngIf="filterable" class="button-filter" [matMenuTriggerFor]="filterMenu" mat-icon-button>
+				<mat-icon svgIcon="filter"></mat-icon>


Could you please add a tooltip ?

SuperITMan · 2018-07-16T06:43:35Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	 * Name of the property that will be the source of the column.
+	 */
+	@Input()
+	public get name(): string {


I guess this implementation replaces ngOnChanges(simplesChanges: SimpleChanges).
Is it working in the same way ?

Indeed, it has the same functionality. It is done like this for the moment because this is just the initial implementation of the table, we will see if we need to refactor this when we implement the rest of functionalities..

SuperITMan · 2018-07-16T07:05:56Z

packages/stark-ui/src/modules/table/components/table.component.html

+<table #matTable mat-table [dataSource]="dataSource" [ngClass]="{'multi-sorting': isMultiSorting}">
+	<ng-container *ngIf="multiselect" matColumnDef="select">
+		<th mat-header-cell *matHeaderCellDef>
+			<mat-checkbox (change)="$event ? masterToggle() : null" [checked]="selection.hasValue() && isAllSelected()"


Could you please add a tooltip ?

?? where exactly?

On the th or, ideally, on the mat-checkbox element. But I'm not sure we can add tooltip directly on mat-checkbox component

I see, ok, I'll add it 😉

SuperITMan · 2018-07-16T07:10:15Z

packages/stark-ui/src/modules/table/components/table.component.html

+	<tr mat-row *matRowDef="let row; columns: displayedColumns;"></tr>
+</table>
+
+<mat-paginator [pageSizeOptions]="[5, 10, 20]" showFirstLastButtons></mat-paginator>


Previously, it was possible to customize pagination options as those ones. Could you please put this back ?

Nop, pagination is not part of this PR, as I mentioned in the description, this is only about nested data, sorting and filtering. Pagination and other features will be part of another PR 😉

SuperITMan · 2018-07-16T07:12:42Z

packages/stark-ui/src/modules/table/table.module.ts

+import { FormsModule } from "@angular/forms";
+import { BrowserAnimationsModule } from "@angular/platform-browser/animations";
+import { MatButtonModule } from "@angular/material/button";
+import { MatCheckboxModule } from "@angular/material/checkbox";


Why import every Angular Material elements separately instead of together as this:

import { MatCheckboxModule, MatButtonModule, ... }

Good question, in fact I am not sure whether there are still some issues regarding the "tree-shaking" of the material components. See angular/components#4137 and angular/components#6210.

I think we can do some checks to verify it, but I would prefer to do it in another PR...

Okido. My question is there because we didn't split the imports in other UI modules. But I guess we can merge them 😊 But let's do in another PR if you prefer

SuperITMan · 2018-07-16T07:16:55Z

packages/stark-ui/src/modules/table/components/table.component.ts

+	@Input() public filter: StarkTableFilter;
+
+	/**
+	 * FIXME: should be "true" by default


If it should be true, what are we waiting for setting it as true?

Sorry, wrong FIXME, I'll remove it 😉

SuperITMan · 2018-07-16T07:21:11Z

packages/stark-ui/src/modules/table/components/table.component.ts

+			}
+		}
+
+		if (changes["filter"]) {


Could we move this in the @Input declaration as it is the case in column component (with get/set) ?

Why? In this ngOnChanges we handle the updates of 3 inputs: data, orderProperties and filter. Whereas in the ColumnComponent we only handle 1 input: name. In any case, as I said in the other comment of the ColumnComponent, we might refactor the getter/setter in case we need it. But this ngOnChanges block should not change in my opinion

Okido. Let's keep it as this so 😊

catlabs · 2018-07-13T14:49:56Z

packages/stark-ui/package.json

@@ -39,6 +39,7 @@
    "@angular/common": "6.x",
    "@angular/compiler": "6.x",
    "@angular/core": "6.x",
+    "@angular/flex-layout": "6.x",


Flex Layout should not be used

Indeed, it is not yet used. I'll remove it so we will add it in another PR once we really need it 😉

catlabs · 2018-07-13T15:15:25Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	 * @param obj2 - Second object in the comparison
+	 */
+	@Input() public compareFn?: ((obj1: any, obj2: any) => number);
+	@Input() public dataAccessor?: ((data: any, name: string) => string); // TODO: really needed?


Missing JSDoc

catlabs · 2018-07-13T15:15:39Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	 * @param columnName - The column that the cell belongs to
+	 */
+	@Input() public cellFormatter?: ((value: any, row?: any, columnName?: string) => string);
+	@Input() public sortDirection: StarkTableColumnSortingDirection;


Missing JSDoc

catlabs · 2018-07-13T15:16:32Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	 * Use "\*" and "\?" to match exactly the characters "*" and "?"
+	 */
+	@Input() public filterValue: string;
+	@Input() public headerLabel: string;


Missing JSDoc

catlabs · 2018-07-13T15:16:40Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	 * Whether the column is sortable or not. Default: true
+	 */
+	@Input() public sortable: boolean = false;
+	@Input() public sortPriority: number;


Missing JSDoc

catlabs · 2018-07-13T15:16:51Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	 */
+	@Input() public sortable: boolean = false;
+	@Input() public sortPriority: number;
+	@Input() public visibility: number;


Missing JSDoc

catlabs · 2018-07-13T15:16:57Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	@Input() public sortPriority: number;
+	@Input() public visibility: number;
+
+	@Output() public filterChanged: EventEmitter<StarkTableColumnComponent> = new EventEmitter<StarkTableColumnComponent>();


Missing JSDoc

catlabs · 2018-07-13T15:17:03Z

packages/stark-ui/src/modules/table/components/column.component.ts

+	@Input() public visibility: number;
+
+	@Output() public filterChanged: EventEmitter<StarkTableColumnComponent> = new EventEmitter<StarkTableColumnComponent>();
+	@Output() public sortChanged: EventEmitter<StarkTableColumnComponent> = new EventEmitter<StarkTableColumnComponent>();


Missing JSDoc

catlabs · 2018-07-16T07:17:37Z

packages/stark-ui/src/modules/table/components/_table-theme.scss

+  .stark-table table {
+    border-radius: map-get($theme, border-radius);
+  }
+}


I think those styles can be removed. As we discussed, we will only provided styles related to the color and font of the component in the theme SCSS.
For other styles, code should be placed in the _table.component.scss

In this specific case, we won't provide any border radius theme variable. If we want to change the angular material default radius, we should then put our style in the _table.component.scss. The end user can then either use it or not by importing this file.

Ok, I'll remove them 😉

catlabs · 2018-07-16T07:38:40Z

packages/stark-ui/src/modules/table/components/table.component.ts

+			StarkTableMultisortDialogComponent,
+			StarkTableMultisortDialogData
+		>(StarkTableMultisortDialogComponent, {
+			width: "500px",


We should try to set this width through the SCSS, shall we create a ticket for that?

Ok. A ticket is not necessary, I'll fix it 😉

…orting nested data, sorting and filtering)

christophercr · 2018-07-16T10:59:25Z

@catlabs @SuperITMan I've updated this PR. Apart from the changes for yout remarks, I've added a demo page in the showcase for the Table component ;)

Could you please have a look and re-review this PR?

christophercr added type: feature comp: stark-ui labels Jul 12, 2018

christophercr added this to the 10.0.0-alpha.4 milestone Jul 12, 2018

christophercr requested review from catlabs, RobbyDeLaet and SuperITMan July 12, 2018 11:50

christophercr force-pushed the feature/stark-table branch from 58367b6 to d2ad745 Compare July 12, 2018 13:27

christophercr force-pushed the feature/stark-table branch 3 times, most recently from fd73d6a to ee07179 Compare July 13, 2018 14:52

SuperITMan reviewed Jul 16, 2018

View reviewed changes

catlabs suggested changes Jul 16, 2018

View reviewed changes

christophercr force-pushed the feature/stark-table branch from ee07179 to 173a36b Compare July 16, 2018 10:36

feat(stark-ui): initial implementation of Stark Table component (supp…

80e86c3

…orting nested data, sorting and filtering)

christophercr force-pushed the feature/stark-table branch from 173a36b to 80e86c3 Compare July 16, 2018 10:58

christophercr removed the request for review from RobbyDeLaet July 16, 2018 10:59

catlabs approved these changes Jul 16, 2018

View reviewed changes

SuperITMan merged commit 9cbceb7 into NationalBankBelgium:master Jul 16, 2018

christophercr deleted the feature/stark-table branch July 16, 2018 12:59

feat(stark-ui): initial implementation of Stark Table component (supporting nested data, sorting and filtering) #501

feat(stark-ui): initial implementation of Stark Table component (supporting nested data, sorting and filtering) #501

Conversation

christophercr commented Jul 12, 2018 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

christophercr commented Jul 12, 2018

christophercr commented Jul 12, 2018

coveralls commented Jul 12, 2018

christophercr commented Jul 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperITMan Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophercr commented Jul 16, 2018 • edited Loading

christophercr commented Jul 12, 2018 •

edited

Loading

SuperITMan Jul 16, 2018 •

edited

Loading

christophercr commented Jul 16, 2018 •

edited

Loading