feat(stark-ui): action-bar Component #495

catlabs · 2018-07-10T14:23:41Z

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: #481

What is the new behavior?

Action Bar - implement component/module
New stark-action-bar component integrated in stark-ui package.

coveralls · 2018-07-10T14:57:51Z

Coverage decreased (-0.05%) to 94.648% when pulling d1dd733 on catlabs:feature/action-bar into 34af0ae on NationalBankBelgium:master.

RobbyDeLaet · 2018-07-10T14:54:38Z

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

+describe("ActionBarComponent", () => {
+	let fixture: ComponentFixture<StarkActionBarComponent>;
+	let component: StarkActionBarComponent;
+	const buttonToogleSelector: string = ".extend-action-bar";


Just a small typo buttonToogleSelector -> buttonToggleSelector 😏

RobbyDeLaet · 2018-07-10T15:05:20Z

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

+	public isExtended: boolean = false;
+
+	/**
+	 * toogle the extended action in full mode


Same typo toogle => toggle 😉

RobbyDeLaet · 2018-07-10T15:16:05Z

showcase/package.json

@@ -121,8 +121,8 @@
    "@angular/platform-browser-dynamic": "6.0.7",
    "@angular/platform-server": "6.0.7",
    "@angular/router": "6.0.7",
-    "@nationalbankbelgium/stark-core": "file:../dist/packages-dist/stark-core/nationalbankbelgium-stark-core-10.0.0-alpha.3-6796ad0.tgz",
-    "@nationalbankbelgium/stark-ui": "file:../dist/packages-dist/stark-ui/nationalbankbelgium-stark-ui-10.0.0-alpha.3-6796ad0.tgz",
+    "@nationalbankbelgium/stark-core": "file:../dist/packages-dist/stark-core/nationalbankbelgium-stark-core-10.0.0-alpha.3-00a78ab.tgz",


Don't include the package.json files in the commit if only the build number of a stark package has changed.

RobbyDeLaet · 2018-07-10T15:22:57Z

showcase/src/app/router.config.ts

-	router.plugin(Visualizer);
+	//Uncomment if needed
+	/*router.trace.enable(Category.TRANSITION);
+	router.plugin(Visualizer);*/


It is better to put these kind of changes in a separate PR.
Now it is hidden inside this PR and harder to find in the future.

RobbyDeLaet · 2018-07-10T15:24:41Z

showcase/src/app/shared/example-viewer/example-viewer.component.ts

@@ -10,7 +10,7 @@ import { STARK_LOGGING_SERVICE, StarkErrorImpl, StarkLoggingService } from "@nat
 	providers: [FileService]
 })
 export class ExampleViewerComponent implements OnInit {
-	@Input() public extensions: string[] = ["HTML", "TS", "CSS"];


Why is the CSS removed?

For every component, we have now 2 SCSS files, one for the colors (them) and one for the rest of the styles. It is up to the end user if he wants to include those files or not, so they have more flexibility.

Mmm I think that this has nothing to do with the fact that there are 2 SCSS files for each component. This remark is about the CSS tab of the example viewer. Aren't we supposed to demo all what is needed to use a certain component? That means the TS code, HTML and CSS

Well for the action bar component for instance, I don't need any specific CSS

True, but this is the example-viewer component so technically by default a demoed component should have HTML and CSS. In your case, the usage you do for the Action Bar demo you don't need it but that is just a specific use case so you just give a different value in the "extensions" input.

RobbyDeLaet · 2018-07-10T15:28:05Z

starter/package.json

@@ -124,8 +124,8 @@
    "@angular/platform-browser-dynamic": "6.0.7",
    "@angular/platform-server": "6.0.7",
    "@angular/router": "6.0.7",
-    "@nationalbankbelgium/stark-core": "file:../dist/packages-dist/stark-core/nationalbankbelgium-stark-core-10.0.0-alpha.3-6796ad0.tgz",
-    "@nationalbankbelgium/stark-ui": "file:../dist/packages-dist/stark-ui/nationalbankbelgium-stark-ui-10.0.0-alpha.3-6796ad0.tgz",


Same remark:
Don't include the package.json files in the commit if only the build number of a stark package has changed.

christophercr

Apart from the remarks below, I think the translation support is missing. Could you please add it?

You can have a look in the old Stark how this is implemented.

christophercr · 2018-07-11T07:40:43Z

packages/stark-ui/assets/styles/_old-media-queries.scss

@@ -0,0 +1,8 @@
+$tablet-query: "(min-width: 600px)";


Why is this file called "old-media-queries"? Are we going to create new media queries? If not, then I think we should simply call it "media-queries"

christophercr · 2018-07-11T08:06:13Z

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

+					{ provide: STARK_LOGGING_SERVICE, useValue: new MockStarkLoggingService() },
+					{ provide: STARK_ROUTING_SERVICE, useClass: MockStarkRoutingService }
+				],
+				schemas: [NO_ERRORS_SCHEMA] // tells the Angular compiler to ignore unrecognized elements and attributes


Can you please specify in the comment which elements we don't want to include?

christophercr · 2018-07-11T08:09:55Z

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

+				label: "Validate",
+				icon: "check",
+				actionCall: ($event: Event, data: any): void => {
+					console.log($event);


Let's avoid adding console.log in the tests otherwise they are cluttered with lots of logs that makes it hard to read the test logs. You can use an spy or a noop.

Moreover, if you use Spies then in your tests you can remove the spyOn(...) calls since they will already be spies ;)

christophercr · 2018-07-11T08:10:24Z

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

+				label: "Save",
+				icon: "content-save",
+				actionCall: ($event: Event, data: any): void => {
+					console.log($event);


Same remark... let's avoid console.log

christophercr · 2018-07-11T08:11:16Z

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

@@ -0,0 +1,98 @@
+"use strict";


The use strict directive is not needed anymore, this is added automatically by TSC when compiling the code.

christophercr · 2018-07-11T09:02:28Z

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

+			component.mode = 'full';
+			fixture.detectChanges();
+			const buttonToggleExtend: HTMLElement = fixture.nativeElement.querySelector(buttonToggleSelector);
+			expect(buttonToggleExtend).toBeTruthy();


Perhaps the matcher .toBeDefined() is more suitable and accurate in this case

christophercr · 2018-07-11T09:02:53Z

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

+			component.mode = 'compact';
+			fixture.detectChanges();
+			const buttonToggleExtend: HTMLElement = fixture.nativeElement.querySelector(buttonToggleSelector);
+			expect(buttonToggleExtend).toBeFalsy();


Perhaps the matcher .toBeUndefined() is more accurate in this case

christophercr · 2018-07-11T09:10:53Z

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

+	describe("@Input() actionBarId", () => {
+		it("should have set the id of the action bar", () => {
+			const actionBar: HTMLElement = fixture.nativeElement.querySelector("#"+component.actionBarId);
+			component.actionBarId = "action-bar-id";


These 2 lines are a bit weird and confusing. The first line sets already the id (which is unknown by the way at this point) and then the second line sets the id. Finally in the last line of this test the id is checked.

You can improve this test by one of these 2 options:

changing the order of these 2 lines and get the HTML element just after calling fixture.detectChanges()

add an expectation just after the first line to expect the HTMLElement to be undefined (because the querySelector will not find the element) and then add another expectation before the last line to expect the HTMLElement to be defined.

christophercr · 2018-07-11T09:11:28Z

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

+	});
+
+	describe("@Input() actionBarConfig", () => {
+		it("should call the defined action when disabled", () => {


"should NOT call the defined action when disabled" ;)

christophercr · 2018-07-11T09:13:15Z

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

+
+		it("should display", () => {
+			const actionBar: HTMLElement = fixture.nativeElement.querySelector(".open-alt-actions");
+			expect(actionBar).toBeTruthy();


Perhaps the matcher .toBeDefined() is more suitable and accurate in this case

catlabs · 2018-07-11T13:15:07Z

I added a ticket about translation:
#497

dsebastien · 2018-07-13T10:24:31Z

packages/stark-core/src/common/translations/translations/en.ts

@@ -12,6 +12,21 @@ export const translationsEn: object = {
 		SORTING: {
 			ASC: "Ascending",
 			DESC: "Descending"
+		},


We'll have to think about the best place to put those translations.

Issue created: #511

SuperITMan · 2018-07-13T12:32:29Z

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

+export class StarkActionBarComponent {
+	@Input() public actionBarId: string = "";
+	@Input() public actionBarConfig: StarkActionBarConfig;
+	@Input() public actionBarScope: any;


Could you please JSDoc documentation for those bindings ?

ISSUES CLOSED: NationalBankBelgium#481

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

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

catlabs self-assigned this Jul 10, 2018

catlabs requested review from dsebastien, christophercr, SuperITMan and RobbyDeLaet July 10, 2018 14:23

RobbyDeLaet suggested changes Jul 10, 2018

View reviewed changes

catlabs force-pushed the feature/action-bar branch from a61084c to ceff69e Compare July 11, 2018 08:22

christophercr suggested changes Jul 11, 2018

View reviewed changes

RobbyDeLaet approved these changes Jul 11, 2018

View reviewed changes

catlabs force-pushed the feature/action-bar branch from ceff69e to bb9d6ba Compare July 11, 2018 10:58

catlabs force-pushed the feature/action-bar branch 3 times, most recently from c84a611 to ce3fb85 Compare July 12, 2018 10:01

christophercr approved these changes Jul 12, 2018

View reviewed changes

dsebastien approved these changes Jul 13, 2018

View reviewed changes

SuperITMan mentioned this pull request Jul 13, 2018

ui: action bar - think about a border around the bar and the shadow below #509

Open

SuperITMan requested changes Jul 13, 2018

View reviewed changes

catlabs force-pushed the feature/action-bar branch 2 times, most recently from 5fc32cd to 97375e3 Compare July 13, 2018 13:13

feat(stark-ui): action Bar Component

d1dd733

ISSUES CLOSED: NationalBankBelgium#481

catlabs force-pushed the feature/action-bar branch from 97375e3 to d1dd733 Compare July 13, 2018 13:20

christophercr changed the title ~~feat(stark-ui): action Bar Component~~ feat(stark-ui): action-bar Component Jul 13, 2018

christophercr merged commit 90c53fe into NationalBankBelgium:master Jul 13, 2018

catlabs deleted the feature/action-bar branch July 17, 2018 12:33

christophercr mentioned this pull request Jul 17, 2018

ui: action bar - add tests #218

Closed

SuperITMan unassigned catlabs Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stark-ui): action-bar Component #495

feat(stark-ui): action-bar Component #495

catlabs commented Jul 10, 2018 •

edited by christophercr

Loading

coveralls commented Jul 10, 2018 •

edited

Loading

RobbyDeLaet Jul 10, 2018

RobbyDeLaet Jul 10, 2018

RobbyDeLaet Jul 10, 2018

RobbyDeLaet Jul 10, 2018

RobbyDeLaet Jul 10, 2018

catlabs Jul 11, 2018

christophercr Jul 11, 2018

catlabs Jul 11, 2018

christophercr Jul 11, 2018 •

edited

Loading

RobbyDeLaet Jul 10, 2018

christophercr left a comment

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

christophercr Jul 11, 2018

catlabs commented Jul 11, 2018

dsebastien Jul 13, 2018

christophercr Jul 13, 2018

SuperITMan Jul 13, 2018

SuperITMan Jul 13, 2018

feat(stark-ui): action-bar Component #495

feat(stark-ui): action-bar Component #495

Conversation

catlabs commented Jul 10, 2018 • edited by christophercr Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

coveralls commented Jul 10, 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

christophercr Jul 11, 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

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 11, 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

catlabs commented Jul 10, 2018 •

edited by christophercr

Loading

coveralls commented Jul 10, 2018 •

edited

Loading

christophercr Jul 11, 2018 •

edited

Loading