feat(stark-ui): implement the button's theme #517

catlabs · 2018-07-17T09:34:40Z

ISSUES CLOSED: #476

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?

Implementation of the button's theme

Issue Number: #476

What is the new behavior?

The old style have been merged and some new ones have been added (white buttons)
The html will be a bit different but it follows the angular material way of declaring a button

SuperITMan

Small remark

SuperITMan · 2018-07-17T10:23:30Z

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

@@ -4,27 +4,27 @@
 		<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 [ngClass]="action.className" [matTooltip]="action.labelSwitchFunction ? action.labelActivated : action.label" [disabled]="!action.isEnabled" mat-icon-button>
-					<mat-icon starkSvgViewBox [svgIcon]="action.iconSwitchFunction ? action.iconActivated : action.icon"></mat-icon>
+				<button color="primary" [ngClass]="action.className" [matTooltip]="action.labelSwitchFunction ? action.labelActivated : action.label" [disabled]="!action.isEnabled" mat-icon-button>


Why force setting primary color?
Maybe should we provide a new property in the interface to set a color? Which is an optional string and, if it is not set, we can set primary by default. What do you think about this? 😊

SuperITMan · 2018-07-17T10:23:30Z

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

 				</button>
 				<label class="action-label" [ngClass]="{'disabled': !action.isEnabled}" *ngIf="isExtended" translate>{{ action.label }}</label>
 			</ng-container>
 		</div>
 	</div>

 	<div *ngIf="mode === 'full' || alternativeActions" class="alt-actions">
-		<button class="extend-action-bar" mat-icon-button *ngIf="mode === 'full'" (click)="toggleExtendedActionBar()">
-			<mat-icon starkSvgViewBox svgIcon="dots-horizontal"></mat-icon>
+		<button class="extend-action-bar" color="primary" mat-icon-button *ngIf="mode === 'full'" (click)="toggleExtendedActionBar()">


Why force setting primary color?
Maybe should we provide a new property in the interface to set a color? Which is an optional string and, if it is not set, we can set primary by default. What do you think about this? 😊

This is a feature coming from Angular Material, nothing added on our side.

https://material.angular.io/components/button/examples

I think what @SuperITMan is talking about is the fact that we "enforce" the primary color on the action buttons. I'm also not sure if we should enforce such color, what if the developer wants a different color? How can he change the color without affecting the primary color of other buttons?

As I mentioned the other day, I think the safest approach will be not to use the Angular Material color directive in the Stark packages but only in the client apps like the showcase and the starter.

If we want a closest integration with Angular Material, I think we should keep the directive that they provide. Now I have extend the 3 default colors to add the one we were using before. Then the end user could use Angular Material button with our custom colors, I think that provide the most flexibility.

Imagine if we give up on the color attr and they write this:
<button color="primary" class="stark-alert" mat-raised-button>Primary or Alert?</button>
Then this button have 2 styles defined, so one should override the other

With this notation:
<button color="alert" mat-raised-button>Alert</button>
There is no confusion and we stay inline with Angular Material

If we really want to override the color we can do it through a mixin that I have created, which use actually the angular material mixin for buttons.

The code for end user will be:

.my-custom-button { @include stark-button-color(#fff, #222); //(color, text-color) }

@catlabs What do you think if we add a property in StarkAction ?
Now, we have something like this:

export interface StarkAction { // ... isEnabled: boolean; label: string; // ... }

But we could have something else like:

export interface StarkAction { // ... isEnabled: boolean; label: string; buttonColor: string; // <------- // ... }

Then, in the component, we could have this:

<button color="action.buttonColor ? action.buttonColor : 'alert'" mat-raised-button>Alert</button>

What do you think about this ?

We continue to use Angular Material as it should be but we give more abilities to choose the color of the button.
I don't think we should use @mixin function to change the color of the button...

Well yeah that's an idea, we talked about it with @christophercr actually, I ll do that and repush the change

Mmm, but then we should think about what are the possible colors we would accept; any? just primary, accent and alert?

If we accept any, in case a new color is given, then it means that the developer should also "create" that color with the Angular Material mixins which at the end willl be the same effort that the one @catlabs mentioned above in order to override the color.

So, I don't know if this approach of having a buttonColor in the interface will make things easier or not

I think you're right Christopher 😊

We could do so

{ //... buttonColor: "primary" | "alert" | "accent"; }

What do you think about this?
And if they want more flexibility, they will have to play with @mixin function 😊

Well yeah that's the idea, they can either enter one of the seven colors we provide (primary, warn,accent,success,alert,alt,neutral,white) or a custom one, then they will have to play with the mixin

christophercr · 2018-07-17T10:18:52Z

showcase/src/app/app.component.html

@@ -31,7 +42,7 @@
 			</a>
 		</mat-nav-list>
 	</mat-sidenav>
-  <mat-sidenav-content class="px4 py2">
+	<mat-sidenav-content class="px4 py2">


What are these classes for? Where are they defined?

As discussed 2 weeks ago, those classes are coming from basscss. It provides some global classes (padding in this case) which is convenient to avoid having extra css on our side.
Regarding sidenav, we will implement it soon, so this code is going to change.

christophercr · 2018-07-17T10:24:57Z

showcase/src/app/app.module.ts

@@ -154,7 +154,11 @@ export const metaReducers: MetaReducer<State>[] = ENV !== "production" ? [logger
 		StarkRoutingModule.forRoot(),
 		SharedModule,
 		DemoModule,
-		StarkAppLogoModule
+		StarkAppLogoModule,


All these modules (except the StarkAppLogoModule) should not be imported here, they are now imported in the Demo module ;)

christophercr · 2018-07-17T10:26:25Z

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

+	public constructor(@Inject(STARK_LOGGING_SERVICE) public logger: StarkLoggingService) {}
+
+	public ngOnInit(): void {
+		console.log("On Init");


Replace the console.log by the StarkLoggingService since you already injected it ;)

oups my mistake :-)

christophercr · 2018-07-17T10:29:11Z

showcase/src/app/demo/index.ts

@@ -2,3 +2,6 @@ export * from "./action-bar";
 export * from "./example-viewer";
 export * from "./table";
 export * from "./demo.module";
+export * from "./action-bar/action-bar.component";


This re-export is not needed, This is already done in this barrel: demo/action-bar/index.ts

christophercr · 2018-07-17T10:29:43Z

showcase/src/app/demo/index.ts

@@ -2,3 +2,6 @@ export * from "./action-bar";
 export * from "./example-viewer";
 export * from "./table";
 export * from "./demo.module";
+export * from "./action-bar/action-bar.component";
+export * from "./button/button.component";
+export * from "./example-viewer/example-viewer.component";


This re-export is not needed. This is already done in this barrel: demo/example-viewer/index.ts

christophercr · 2018-07-17T10:32:05Z

showcase/src/app/demo/index.ts

@@ -2,3 +2,6 @@ export * from "./action-bar";
 export * from "./example-viewer";
 export * from "./table";
 export * from "./demo.module";
+export * from "./action-bar/action-bar.component";
+export * from "./button/button.component";


This barrel should just re-export everything from ./action-bar. There, inside the "action-bar" folder there should be a barrel index.ts which re-exports all the .ts files except the spec.ts.

See how it is done in the other folders ;)

christophercr · 2018-07-17T10:47:53Z

packages/stark-ui/src/modules/action-bar/components/_action-bar-theme.scss

-  &.stark-action-bar-full {
+}
+
+@media #{$mobile-only-query} {


In the previous code the media query was "tablet-query" and now is "mobile-only-query"... Is this an intended change? Why? What about tablet?

Yes it is intended, now with the button theme, we can specify which color the button should have with the color attribute. It is only an small device that we need to use custom colors, so it make more sense to have SCSS exception on small device

Oh I see, Ok.

christophercr · 2018-07-17T10:50:20Z

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

+import { STARK_LOGGING_SERVICE, StarkLoggingService } from "@nationalbankbelgium/stark-core";
+
+@Component({
+	selector: "showcase-demo-button",


I'm not sure these lines are correctly formatted (indentation) ... could you please run the "prettier-check" command to auto format all files?

Yes I already did it, so I guess the indentation is correct. Seems ok in my editor...

catlabs · 2018-07-17T12:21:17Z

Allright, I repushed the changes. @christophercr @SuperITMan can you recheck?

catlabs · 2018-07-18T09:43:15Z

Changes pushed. @christophercr @SuperITMan can you recheck?

christophercr

Some small remarks

christophercr · 2018-07-18T09:49:59Z

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

 			</ng-container>
 		</div>
 	</div>

 	<div *ngIf="mode === 'full' || alternativeActions" class="alt-actions">
-		<button class="extend-action-bar" mat-icon-button *ngIf="mode === 'full'" (click)="toggleExtendedActionBar()">
-			<mat-icon starkSvgViewBox svgIcon="dots-horizontal"></mat-icon>
+		<button class="extend-action-bar" color="primary" mat-icon-button *ngIf="mode === 'full'" (click)="toggleExtendedActionBar()">


I think this "color" should also be set via the buttonColor right?

Well this button are not set via any Input property, it is the generic extend button. I don't think we should add an Input for this, it will make the api more complex.

christophercr · 2018-07-18T09:50:15Z

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

 		</button>
-		<button class="open-alt-actions" mat-icon-button *ngIf="alternativeActions" [matMenuTriggerFor]="menu">
-			<mat-icon starkSvgViewBox svgIcon="dots-vertical"></mat-icon>
+		<button class="open-alt-actions" color="primary" mat-icon-button *ngIf="alternativeActions" [matMenuTriggerFor]="menu">


Same here, I think this "color" should also be set via the buttonColor right?

Same remark than above

christophercr · 2018-07-18T09:56:48Z

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

+	/**
+	 * Color of the button (optional)
+	 */
+	buttonColor?: string;


The "(optional)" string in the comment is not needed since it is already implicit in the Typescript code. Then, I think the type of this property should be more specific as @SuperITMan suggested, so it would be something like this:

/** * Color of the action button. Default: "primary". */ buttonColor?: "primary" | "alert" | "accent" | "anyOtherStarkColor" | string;

In this case, the "string" type is still needed so the developer can give any other color.

catlabs · 2018-07-18T10:35:01Z

Changes pushed. @christophercr @SuperITMan can you recheck?

SuperITMan · 2018-07-18T13:43:49Z

@catlabs could you please rebase your PR? 😊

ISSUES CLOSED: NationalBankBelgium#476

catlabs · 2018-07-18T13:50:56Z

Rebase pushed

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

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

catlabs requested review from dsebastien, christophercr and SuperITMan July 17, 2018 09:34

SuperITMan reviewed Jul 17, 2018

View reviewed changes

christophercr suggested changes Jul 17, 2018

View reviewed changes

catlabs force-pushed the feature/button branch from 4cd436b to 7be2cff Compare July 17, 2018 12:20

catlabs force-pushed the feature/button branch from 7be2cff to ea1db01 Compare July 18, 2018 09:42

christophercr reviewed Jul 18, 2018

View reviewed changes

catlabs force-pushed the feature/button branch from ea1db01 to 73d28ea Compare July 18, 2018 10:34

catlabs force-pushed the feature/button branch 2 times, most recently from ccf1384 to 240f6f0 Compare July 18, 2018 12:24

christophercr approved these changes Jul 18, 2018

View reviewed changes

SuperITMan approved these changes Jul 18, 2018

View reviewed changes

feat(stark-ui): implementation of the button's theme

c49b6ef

ISSUES CLOSED: NationalBankBelgium#476

catlabs force-pushed the feature/button branch from 240f6f0 to c49b6ef Compare July 18, 2018 13:50

christophercr changed the title ~~feat(stark-ui): implementation of the button's theme~~ feat(stark-ui): implement the button's theme Jul 18, 2018

christophercr merged commit 0a5f663 into NationalBankBelgium:master Jul 18, 2018

catlabs deleted the feature/button branch July 30, 2018 13:48

feat(stark-ui): implement the button's theme #517

feat(stark-ui): implement the button's theme #517

Conversation

catlabs commented Jul 17, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

SuperITMan 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

christophercr Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

catlabs Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

catlabs Jul 17, 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

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 17, 2018

catlabs commented Jul 18, 2018

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

catlabs commented Jul 18, 2018

SuperITMan commented Jul 18, 2018 • edited Loading

catlabs commented Jul 18, 2018

christophercr Jul 17, 2018 •

edited

Loading

catlabs Jul 17, 2018 •

edited

Loading

catlabs Jul 17, 2018 •

edited

Loading

SuperITMan commented Jul 18, 2018 •

edited

Loading