feat(stark-ui): add support for click callbacks and row(s) selection to Table component #870

carlo-nomes · 2018-11-20T11:45:04Z

ISSUES CLOSED: #795 #880

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?
Enable (rowClick) on Table component

[ ] 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?

Clicking on a row does nothing
Issue Number: #795 #880

What is the new behavior?

added (rowClick) emitter to handle a row click so that an observer can be passed to the emitter for custom behaviour.
set default row click to selecting the row
fixed selecting rows
fixed action buttons propagation
added hover styling to rows
added test for row click
added example to demo page

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

coveralls · 2018-11-20T14:05:05Z

Coverage decreased (-0.05%) to 93.523% when pulling 0182344 on cnomes:feature/table-row-click into 110901b on NationalBankBelgium:master.

packages/stark-ui/src/modules/action-bar/components/action-bar.component.ts

SuperITMan · 2018-11-20T15:06:38Z

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

+		}
+
+		// If multi-select is enabled, select the row
+		if (this.isMultiSelectEnabled) {


Previously, if the developer set an action on the row, we didn't select the row. It was the job of the dev if he wanted to select it... I think maybe it should stay as this, otherwise it will become really complex to understand what's happening when we click 😕

What do you think about this ?
@christophercr @dsebastien ? 😊

I did do this, but I terminated the function if an event observer is found.
I could also do this with else if if it is preferred.

Indeed, I agree with @SuperITMan, we should keep the old behaviour, but as far as I can see it is already the case in this implementation right?

By the way, the rowsSelectable input is missing in this component... it is there in the old Stark... so the row will be selected only if rowsSelectable is enabled.

Actually, to be honest, I didn't see the returns :-p
Anyways, I prefer to have if, else if than return. I find it more readable 😊

christophercr

Some remarks. Also it would be convenient to check whether we can rely a bit more on the SelectionModel from the MaterialTable which we use in this variable:
public selection: SelectionModel<any> = new SelectionModel<any>(true, []);
so we can keep it in sync with our multiselect input.

christophercr · 2018-11-20T16:34:34Z

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

@@ -83,6 +83,10 @@
      }
    }

+    tr:hover td {
+      background: rgba(0, 0, 0, 0.05);


I think this rule should be in the _table-theme.scss file. Normally as a convention we put all the styles about "themable" properties (i.e. colors that the developer can override) in those theme files.

You can check other components where we have these "theme" files to see how you can get a color from the current theme ;)

christophercr · 2018-11-20T16:38:34Z

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

@@ -38,7 +38,7 @@

 <div [ngClass]="{'fixed-header': isFixedHeaderEnabled}">
 	<table #matTable mat-table [dataSource]="dataSource" [ngClass]="{'multi-sorting': isMultiSorting}">
-		<ng-container *ngIf="isMultiSelectEnabled" matColumnDef="select">
+		<ng-container matColumnDef="select">


Shouldn't the "select all" checkbox be visible only when the multiselect option is enabled?
In fact, in the old Stark the "select" checkbox was shown in every row when the rows-selectable input was enabled...

I agree with you. If you multiSelect is not enabled, we shouldn't have the checkbox to select all...

I added a check below to only render the header checkbox if the multiSelect is added, the showing of the other checkboxes is depends on the state of rowsSelectable, but is controlled with the dispalyedColumns field.

christophercr · 2018-11-20T16:47:09Z

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

+	 * If there are no observers it will not emit, but instead select the row.
+	 */
+	@Output()
+	public rowClick: EventEmitter<any> = new EventEmitter<any>();


As a convention, the name of the outputs should be in past tense like "rowClicked"

christophercr · 2018-11-20T16:48:15Z

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

+	 * If there are no observers it will not emit, but instead select the row.
+	 */
+	@Output()
+	public rowClick: EventEmitter<any> = new EventEmitter<any>();


I think this EventEmitter should emit the row object according to the API from the old Stark

It does, doesn't it? 🤔

Yes, I just meant the type of this property: "public rowClick: EventEmitter<object> = new EventEmitter<object>();"

Ah ok, I understand. Should I also change the other types referring to the row?

christophercr · 2018-11-20T16:51:15Z

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

-			if (this.isMultiSelectEnabled && this.displayedColumns) {
+		if (changes["multiSelect"] && this.isMultiSelectEnabled) {
+			if (!this.displayedColumns) {
+				this.displayedColumns = [];


I think this block of code should not be here, it should be somewhere else but not here since this is only for checking changes on the "multiselect" input, otherwise out code will get very confusing and difficult to troubleshoot in case of errors. Moreover, the displayedColumns property is already initialized in the property declaration.

I changed the code in this block a bit, but there is still some logic about multiselect there. I could move it to a setter. But there would be some weird logic because it is a string treated as a boolean.
I'm watching for this issue but I doubt it will be picked up up anytime soon.

Ah this is fine, I was referring to the fact that the displayedColumns were initialized here as well, but you have changed this already 👍

christophercr · 2018-11-20T17:01:35Z

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

+		}
+
+		// If multi-select is enabled, select the row
+		if (this.isMultiSelectEnabled) {


Indeed, I agree with @SuperITMan, we should keep the old behaviour, but as far as I can see it is already the case in this implementation right?

By the way, the rowsSelectable input is missing in this component... it is there in the old Stark... so the row will be selected only if rowsSelectable is enabled.

christophercr · 2018-11-20T17:04:55Z

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

@@ -171,6 +171,13 @@ export class StarkTableComponent extends AbstractStarkUiComponent implements OnI
 	@Output()
 	public selectChanged: EventEmitter<number[]> = new EventEmitter<number[]>();


I think this input is not used at all for now... but it should right? This should emit whenever a row is selected as in the old Stark (on-select)

christophercr · 2018-11-20T17:10:52Z

packages/stark-ui/src/modules/action-bar/components/action-bar.component.ts

@@ -100,6 +100,9 @@ export class StarkActionBarComponent extends AbstractStarkUiComponent implements
 	 * Action onClick handler
 	 */
 	public onClick(action: StarkAction, $event: Event): void {
+		if ($event) {
+			$event.stopPropagation();


Can you add a comment here to explain why this is needed?

christophercr · 2018-11-20T17:13:55Z

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

@@ -83,6 +83,10 @@
      }
    }

+    tr:hover td {


We need to set a background in the selected rows as well. This is not only in the old Stark but also in the Material Design specs: https://material.io/design/components/data-tables.html#anatomy -> Row Checkbox

christophercr · 2018-11-22T11:05:51Z

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

-		this.displayedColumns = [];
+
+		// Emit event when selection changes
+		if (this.selection.changed) {


Is this check really needed? Normally this.selection is initialized in its declaration and the "changes" observable should always be there right?

Indeed, I don't remember why I did that. 🤔

christophercr · 2018-11-22T11:11:33Z

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

 			}
 		}
+
+		if (changes["multiSelect"]) {
+			this.selection = new SelectionModel<object>(this.isMultiSelectEnabled, []);


I think we might have a problem here: we re-assign the SelectionModel here meaning that the subscription we did on the ngOnInit will still be on the old SelectionModel right?

You could extract the code to initialize the SelectionModel and its subscription to a separate function which you can call on the ngOnInit and here...

christophercr · 2018-11-22T11:18:09Z

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

-			if (this.isMultiSelectEnabled && this.displayedColumns) {
+		if (changes["multiSelect"] && this.isMultiSelectEnabled) {
+			if (!this.displayedColumns) {
+				this.displayedColumns = [];


Ah this is fine, I was referring to the fact that the displayedColumns were initialized here as well, but you have changed this already 👍

christophercr

Small remarks

christophercr · 2018-11-22T11:23:45Z

showcase/src/app/demo/table/components/table-with-selection/table-with-selection.ts

+	}
+
+	public handleRowSelected(data: object[]): void {
+		this.logger.debug(data);


Can you add an string before the data so we know in the console what this logging is about?

christophercr · 2018-11-22T11:34:46Z

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

+  tr {
+    &:hover td {
+      background-color: mat-color($grey-palette, 400);
+      color: mat-contrast($grey-palette, 400);


Can we set a lighter gray on hover? And also.. can we leave the color of the text unchanged?
Basically as it was in the old Stark and I think this is also what the Material Design guideline suggests...

christophercr · 2018-11-22T11:41:10Z

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

+		if (this.selection.changed) {
+			this._selectionSub = this.selection.changed.subscribe((change: SelectionChange<object>) => {
+				const selected: object[] = change.source.selected;
+				this.selectChanged.emit(selected);


Now that we use the SelectionModel to handle the selection, the array we emit is an array of objects (rows) while in the old Stark it was an array of ids. So could you add this API change in the Confluence page please about Stark 9 vs Stark 10?

carlo-nomes · 2018-11-22T13:57:46Z

@christophercr I updated the PR.

- added event emitter to handle a row click - set default row click to selecting the row - fixed selecting rows - fixed action buttons propagation - added hover styling to rows - added test for row click - added example to demo page - fixed issue with double scrollbar - fixed issue with extra padding - added demo for table with selection - added test for selection in table - changed type of rows from any to object ISSUES CLOSED: NationalBankBelgium#795

carlo-nomes requested review from christophercr and SuperITMan November 20, 2018 11:45

carlo-nomes force-pushed the feature/table-row-click branch 2 times, most recently from 7bb032a to 5742013 Compare November 20, 2018 13:28

SuperITMan reviewed Nov 20, 2018

View reviewed changes

carlo-nomes force-pushed the feature/table-row-click branch from 5742013 to 7e6b017 Compare November 20, 2018 15:33

christophercr suggested changes Nov 20, 2018

View reviewed changes

carlo-nomes force-pushed the feature/table-row-click branch 5 times, most recently from 1d98688 to 45df506 Compare November 22, 2018 09:35

carlo-nomes added type: feature comp: stark-ui labels Nov 22, 2018

carlo-nomes force-pushed the feature/table-row-click branch 2 times, most recently from ea4e0a4 to c84cafd Compare November 22, 2018 09:59

christophercr reviewed Nov 22, 2018

View reviewed changes

christophercr suggested changes Nov 22, 2018

View reviewed changes

carlo-nomes force-pushed the feature/table-row-click branch 2 times, most recently from 552961a to f298e3a Compare November 22, 2018 12:53

carlo-nomes force-pushed the feature/table-row-click branch from f298e3a to 99b66f3 Compare November 22, 2018 14:40

carlo-nomes force-pushed the feature/table-row-click branch from 99b66f3 to 0182344 Compare November 22, 2018 14:44

christophercr approved these changes Nov 22, 2018

View reviewed changes

christophercr merged commit b84f8e5 into NationalBankBelgium:master Nov 22, 2018

christophercr mentioned this pull request Nov 22, 2018

ui: table - add support for row selection (single and multiple) #880

Closed

christophercr changed the title ~~feat(stark-ui): add rowClick event emitter to table component~~ feat(stark-ui): add support for click callbacks and row(s) selection to Table component Nov 22, 2018

christophercr added this to the 10.0.0-beta.2 milestone Nov 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stark-ui): add support for click callbacks and row(s) selection to Table component #870

feat(stark-ui): add support for click callbacks and row(s) selection to Table component #870

carlo-nomes commented Nov 20, 2018 •

edited by christophercr

Loading

coveralls commented Nov 20, 2018 •

edited

Loading

SuperITMan Nov 20, 2018

carlo-nomes Nov 20, 2018

christophercr Nov 20, 2018

SuperITMan Nov 20, 2018

christophercr left a comment

christophercr Nov 20, 2018

christophercr Nov 20, 2018

SuperITMan Nov 21, 2018

carlo-nomes Nov 22, 2018

christophercr Nov 20, 2018

christophercr Nov 20, 2018

carlo-nomes Nov 21, 2018

christophercr Nov 21, 2018 •

edited

Loading

carlo-nomes Nov 21, 2018

christophercr Nov 20, 2018

carlo-nomes Nov 22, 2018

christophercr Nov 22, 2018

christophercr Nov 20, 2018

christophercr Nov 20, 2018

christophercr Nov 20, 2018

christophercr Nov 20, 2018

christophercr Nov 22, 2018

carlo-nomes Nov 22, 2018

christophercr Nov 22, 2018 •

edited

Loading

christophercr Nov 22, 2018

christophercr left a comment

christophercr Nov 22, 2018

christophercr Nov 22, 2018

christophercr Nov 22, 2018

carlo-nomes commented Nov 22, 2018

		@@ -171,6 +171,13 @@ export class StarkTableComponent extends AbstractStarkUiComponent implements OnI
		@Output()
		public selectChanged: EventEmitter<number[]> = new EventEmitter<number[]>();

feat(stark-ui): add support for click callbacks and row(s) selection to Table component #870

feat(stark-ui): add support for click callbacks and row(s) selection to Table component #870

Conversation

carlo-nomes commented Nov 20, 2018 • edited by christophercr Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

coveralls commented Nov 20, 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

christophercr left a 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 Nov 21, 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

christophercr Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophercr left a 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

carlo-nomes commented Nov 22, 2018

carlo-nomes commented Nov 20, 2018 •

edited by christophercr

Loading

coveralls commented Nov 20, 2018 •

edited

Loading

christophercr Nov 21, 2018 •

edited

Loading

christophercr Nov 22, 2018 •

edited

Loading