feat(stark-ui): add stark-action-bar into stark-table #525

catlabs · 2018-07-18T07:23:21Z

ISSUES CLOSED: #512

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?

Add the stark-action-bar to the stark-table

Issue Number: #512

christophercr

Apart from integrating the action bar component in the row action bar, we should also integrate it in the table action bar.

catlabs · 2018-07-19T08:21:01Z

Now we also have the main action bar on the table.
I also cleaned up a bit the SCSS import of the table to be inline with the other components.
Action bar has a new Input, buttonColor which set the default color for all buttons.
I added 3 examples on the demo but we still need to add the example viewer (#514)

SuperITMan · 2018-07-19T08:54:46Z

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


    .actions {
      align-items: center;
      display: flex;
      justify-content: flex-end;
+      &.actions-alt{


Could you please add a space before the bracket ?
(Prettier should do it automatically normally 😕)

Strange cause I ran prettier-check before pushing... Maybe we should have a ticket about it?

SuperITMan · 2018-07-19T08:54:49Z

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

 .stark-table {
  .header {
    align-items: center;
    display: flex;
    justify-content: space-between;
+    margin-bottom: 20px;
+
+    .transcluded{


Could you please add a space before the bracket ?
(Prettier should do it automatically normally 😕)

SuperITMan · 2018-07-19T08:55:30Z

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

@@ -4,10 +4,10 @@
 		<div *ngFor="let action of actionBarConfig.actions; trackBy: trackAction" [id]="actionBarId + '-' + action.id"
 		(click)="onClick(action, $event)" class="stark-action-bar--action">
 			<ng-container *ngIf="action.isVisible !== false">
-				<button [color]="action.buttonColor ? action.buttonColor:'primary'" [ngClass]="action.className" [matTooltip]="action.labelSwitchFunction ? action.labelActivated : action.label" [disabled]="!action.isEnabled" mat-icon-button>
+				<button [color]="action.buttonColor ? action.buttonColor:buttonColor" [ngClass]="action.className" [matTooltip]="action.labelSwitchFunction ? action.labelActivated : action.label" [disabled]="!action.isEnabled" mat-icon-button>


Could you please add a space before and after the : for having (action.buttonColor ? action.buttonColor : buttonColor) ?
(Prettier should do it automatically normally 😕)

Well, in fact Prettier doesn't support HTML yet 😩
https://prettier.io/docs/en/language-support.html
prettier/prettier#1882

SuperITMan · 2018-07-19T08:56:02Z

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

 					<mat-icon starkSvgViewBox [svgIcon]="action.iconSwitchFunction ? action.iconActivated : action.icon" class="stark-small-icon"></mat-icon>
 				</button>
-				<label [class]="'action-label '+ (action.buttonColor?action.buttonColor:'primary')" [ngClass]="{'disabled': !action.isEnabled}" *ngIf="isExtended"> <span translate>{{ action.label }}</span></label>
+				<label [class]="'action-label '+ (action.buttonColor ? action.buttonColor:buttonColor)" [ngClass]="{'disabled': !action.isEnabled}" *ngIf="isExtended"> <span translate>{{ action.label }}</span></label>


Could you please add a space before and after the : for having (action.buttonColor ? action.buttonColor : buttonColor) ?
(Prettier should do it automatically normally 😕)

SuperITMan · 2018-07-19T08:58:19Z

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

-	<div class="actions">
-		<button *ngIf="isMultiSortEnabled" (click)="openMultiSortDialog()" mat-icon-button>
-			<mat-icon svgIcon="sort" matTooltip="Multi-Column Sorting"></mat-icon>
+<div class="header mat-el">


What is mat-el class ?

SuperITMan · 2018-07-19T09:01:12Z

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

+	/**
+	 * Type of StarkAction objects
+	 */
+	@Input() public customTableActionsType: 'regular' | 'alt' | string = 'regular';


Could you please replace ' by " ?
(Prettier should do it automatically normally 😕)

christophercr · 2018-07-19T10:04:46Z

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

+		<ng-content select="header"></ng-content>
+	</div>
+	<div class="actions" [ngClass]="{'actions-alt': customTableActionsType === 'alt'}">
+		<stark-action-bar [actionBarConfig]="customTableActionsType === 'regular' ?{actions: customTableActions}:{}" [alternativeActions]="customTableActionsType === 'alt' ?customTableActions:null" buttonColor="alt" mode="compact"></stark-action-bar>


We should avoid adding to much logic in the templates and do all manipulations in the component class instead.

Moreover, in this particular case we are creating new objects which should implement a certain interface, but this is easy to overlook and we might forget to change this whenever we change the interface. This will be much easier to detect in the component class.

Same remark as below:
Well yeah we could have a more flexible approach but this one is the one that cost the less effort regarding the old API.

I understand the reason of this code, but the point here is that this code should be in the component class instead of the template. It is safer there and more difficult to overlook potential issues related to the interface that should be implemented.

christophercr · 2018-07-19T10:06:21Z

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

-			<span>{{ context.displayedValue }}</span>
-		</ng-template>
-	</stark-table-column>
+<mat-card-content>


Why do we use a <mat-card-content> here? I would suggest to use all the minimal MatCard elements or none of them but not just one of them which might cause some weird behavior if used alone.

Maybe a simple <div> with some styling will be better.

Doesn't matter as we will implement the example-viewer #514

Ok, so I think we should just remove the <mat-card-content> and keep the code clean (so we don't need to import the MatCardModule).

christophercr · 2018-07-19T10:12:32Z

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

@@ -24,6 +27,7 @@ import { StarkTableMultisortDialogComponent } from "./components/dialogs/multiso
 		BrowserAnimationsModule,
 		FormsModule,
 		MatButtonModule,
+		MatCardModule,


If we replace the <mat-card-content> in the template, then this will not be needed anymore

Doesn't matter as we will implement the example-viewer #514

christophercr · 2018-07-19T10:12:38Z

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

+	/**
+	 * Type of StarkAction objects
+	 */
+	@Input() public customTableActionsType: "regular" | "alt" | string = "regular";


Why there is the "string" type? Normally, there are only 2 possible values: "regular" and "alt" right?

And for the type "alt", it stands for "alternative" right? Then I think we should use the full name so it is clear the intention. The more descriptive our API is the better.

christophercr · 2018-07-19T10:14:03Z

showcase/src/app/demo/table/table.component.ts

-	templateUrl: "./table.component.html"
+	styleUrls: ["./table.component.scss"],
+	templateUrl: "./table.component.html",
+	encapsulation: ViewEncapsulation.None


Can you please add a comment indicating why you use ViewEncapsulation.None?

christophercr · 2018-07-19T10:15:20Z

showcase/src/styles/_theme.scss

@@ -5,7 +5,8 @@
 @import "~@nationalbankbelgium/stark-ui/assets/themes/button-theme";
 @import "~@nationalbankbelgium/stark-ui/assets/themes/card-theme";
 @import "~@nationalbankbelgium/stark-ui/assets/themes/menu-theme";
-@import "~@nationalbankbelgium/stark-ui/assets/themes/table-theme";
+@import "~@nationalbankbelgium/stark-ui/src/modules/table/components/table.component";
+@import "~@nationalbankbelgium/stark-ui/src/modules/table/components/dialogs/multisort.component";


Why these 2 files are imported here? Normally only the "theme" files should be imported here and not the "component" files.

Well I don't agree, all the other theme files are imported here (see action bar and slider) and I think it is the right place to put them. Here the enduser can decide either he wants to import them or not.

Ah indeed, I overlooked that ;)

christophercr · 2018-07-19T10:15:55Z

showcase/src/styles/styles.scss

@@ -3,6 +3,3 @@
 @import "~basscss/css/basscss.css";
 @import "~normalize.css/normalize.css";
 @import "~prismjs/themes/prism-okaidia.css";
-
-@import "~@nationalbankbelgium/stark-ui/src/modules/table/components/table.component";
-@import "~@nationalbankbelgium/stark-ui/src/modules/table/components/dialogs/multisort.component";


I think these should not be removed from here, see my previous comment

Same remark as above

christophercr · 2018-07-19T10:21:34Z

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

+	<div class="transcluded">
+		<ng-content select="header"></ng-content>
+	</div>
+	<div class="actions" [ngClass]="{'actions-alt': customTableActionsType === 'alt'}">


I'm wondering if this approach (defining the "customTableActionsType" in the table) is flexible enough.

Wouldn't it be more flexible to define it in the Action instead of in the table? Then the developer can decide for every action if they should be "alternative" or not.

Well yeah we could have a more flexible approach but this one is the one that cost the less effort regarding the old API. The idea behind is to either have a full bar or an alt action bar. It won't make sense for now to use both of them in the main table action bar.
For now it is a matter of showing to the end user what we could do and how the interface could be improved, I think we should first propose this option and then see what we do according to their feedbacks.

catlabs · 2018-07-19T12:55:02Z

Re-pushed

SuperITMan · 2018-07-24T11:50:04Z

@catlabs could you rebase your PR ? 😊

ISSUES CLOSED: NationalBankBelgium#512

catlabs · 2018-07-24T12:16:39Z

Yep it is rebased :-)

catlabs added must type: feature comp: stark-ui labels Jul 18, 2018

catlabs added this to the 10.0.0-alpha.4 milestone Jul 18, 2018

catlabs requested review from dsebastien, christophercr and SuperITMan July 18, 2018 07:23

catlabs force-pushed the feature/table-action-bar branch from c38198a to 196a412 Compare July 18, 2018 07:24

christophercr suggested changes Jul 18, 2018

View reviewed changes

catlabs force-pushed the feature/table-action-bar branch 3 times, most recently from eeb1f68 to 44b2e2b Compare July 19, 2018 08:20

SuperITMan requested changes Jul 19, 2018

View reviewed changes

catlabs force-pushed the feature/table-action-bar branch from 44b2e2b to 91a895b Compare July 19, 2018 09:18

christophercr suggested changes Jul 19, 2018

View reviewed changes

catlabs force-pushed the feature/table-action-bar branch 2 times, most recently from f972578 to 6612d19 Compare July 19, 2018 12:54

catlabs force-pushed the feature/table-action-bar branch from 6612d19 to 0adbc00 Compare July 19, 2018 13:27

christophercr added the PR action: review label Jul 23, 2018

dsebastien approved these changes Jul 24, 2018

View reviewed changes

feat(stark-ui): add stark-action-bar into stark-table

d3a2c6d

ISSUES CLOSED: NationalBankBelgium#512

catlabs force-pushed the feature/table-action-bar branch from 0adbc00 to d3a2c6d Compare July 24, 2018 12:15

christophercr approved these changes Jul 24, 2018

View reviewed changes

SuperITMan approved these changes Jul 24, 2018

View reviewed changes

SuperITMan merged commit bd2e5f5 into NationalBankBelgium:master Jul 24, 2018

SuperITMan added this to Done in 10.0.0-alpha.4 Jul 30, 2018

catlabs deleted the feature/table-action-bar branch July 30, 2018 13:48

christophercr removed the PR action: review label Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stark-ui): add stark-action-bar into stark-table #525

feat(stark-ui): add stark-action-bar into stark-table #525

catlabs commented Jul 18, 2018

christophercr left a comment

catlabs commented Jul 19, 2018

SuperITMan Jul 19, 2018

catlabs Jul 19, 2018

SuperITMan Jul 19, 2018

SuperITMan Jul 19, 2018

christophercr Jul 19, 2018

SuperITMan Jul 19, 2018

SuperITMan Jul 19, 2018

SuperITMan Jul 19, 2018

christophercr Jul 19, 2018

catlabs Jul 19, 2018

christophercr Jul 19, 2018

christophercr Jul 19, 2018

catlabs Jul 19, 2018

christophercr Jul 19, 2018

christophercr Jul 19, 2018

catlabs Jul 19, 2018

christophercr Jul 19, 2018 •

edited

christophercr Jul 19, 2018

christophercr Jul 19, 2018

catlabs Jul 19, 2018

christophercr Jul 19, 2018

christophercr Jul 19, 2018

catlabs Jul 19, 2018

christophercr Jul 19, 2018

catlabs Jul 19, 2018

catlabs commented Jul 19, 2018

SuperITMan commented Jul 24, 2018

catlabs commented Jul 24, 2018

feat(stark-ui): add stark-action-bar into stark-table #525

feat(stark-ui): add stark-action-bar into stark-table #525

Conversation

catlabs commented Jul 18, 2018

PR Checklist

PR Type

What is the current behavior?

christophercr left a comment

Choose a reason for hiding this comment

catlabs commented Jul 19, 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

christophercr Jul 19, 2018 • edited

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

catlabs commented Jul 19, 2018

SuperITMan commented Jul 24, 2018

catlabs commented Jul 24, 2018

christophercr Jul 19, 2018 •

edited