feat(stark-ui): implementation of the app sidebar #638

catlabs · 2018-08-27T09:58:14Z

ISSUES CLOSED: #592

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: #592 #569 #534

What is the new behavior?

Implementation of the 2 app sidebars
Note: Only 2 sidebars are allowed in material: angular/components#1514

Does this PR introduce a breaking change?

[ ] Yes
[x] No

coveralls · 2018-08-27T10:19:21Z

Coverage decreased (-0.1%) to 92.4% when pulling 09eeeea on catlabs:feature/sidebar into a444bb0 on NationalBankBelgium:master.

SuperITMan · 2018-08-28T15:10:10Z

@catlabs Could you please revert changes you made in the different package-lock.json files ? 😊

SuperITMan · 2018-08-28T15:11:10Z

packages/stark-ui/assets/themes/_menu-theme.scss

@@ -11,3 +11,24 @@
    color: map-get($mat-light-theme-foreground, disabled-button);
  }
 }
+
+/* TODO: This code soul be moved when the app menu is implemented */


typo issue --> "This code should be ..."

SuperITMan · 2018-08-28T15:43:50Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.ts

+	encapsulation: ViewEncapsulation.None
+})
+export class StarkAppSidebarComponent implements OnDestroy {
+	/**


Could you please add the following piece of code below ?

/** * Adds class="stark-app-sidebar" attribute on the host component */ @HostBinding("class") public class: string = componentName;

SuperITMan · 2018-08-29T10:04:40Z

packages/stark-ui/src/modules/app-sidebar/components/_app-sidebar.component.scss

+/* ============================================================================== */
+/* stark-ui: src/modules/app-sidebar/components/_app-sidebar.component.scss */
+
+stark-app-sidebar {


As proposed below, if we define a class for the component, it would be maybe better to use the class .stark-app-sidebar instead of the tag.

SuperITMan · 2018-08-29T11:59:05Z

showcase/src/app/app.component.html

+		</div>
+	</ng-container>
+	<ng-container stark-app-sidenav-right>
+		<div class="stark-app-sidenav-top">


As we discussed, could you please adapt this element by adding those classes: m1 center ?

SuperITMan · 2018-08-29T11:59:10Z

showcase/src/app/app.component.html

+		<div class="stark-app-sidenav-top">
+			Top content
+		</div>
+		<div class="stark-app-sidenav-middle">


As we discussed, could you please adapt this element by adding those classes: m1 center ?

SuperITMan · 2018-08-29T12:08:33Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.ts

+				break;
+			case "right":
+				from(this.appSidenavRight.open()).subscribe(
+					(result: MatDrawerToggleResult) => this.logger.debug("right sidebar " + result),


Could you please adapt the debug message to be more explicit about the origin of the message ?

this.logger.debug(componentName + ": right sidenav " + result)

SuperITMan · 2018-08-29T12:09:39Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.ts

+			case "left":
+				from(this.appSidenavLeft.open()).subscribe(
+					(result: MatDrawerToggleResult) => this.logger.debug("left sidenav " + result),
+					(error: Error) => this.logger.warn(error)


Could you please adapt the debug message to be more explicit about the origin of the message ?

this.logger.warn(componentName + ": ", error)

SuperITMan · 2018-08-29T12:09:43Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.ts

+			case "right":
+				from(this.appSidenavRight.open()).subscribe(
+					(result: MatDrawerToggleResult) => this.logger.debug("right sidebar " + result),
+					(error: Error) => this.logger.warn(error)


Could you please adapt the debug message to be more explicit about the origin of the message ?

this.logger.warn(componentName + ": ", error)

SuperITMan · 2018-08-29T12:13:52Z

packages/stark-ui/src/modules/app-sidebar/services/app-sidebar.service.ts

+ * StarkAppSidebarService service
+ */
+@Injectable()
+export class StarkAppSidebarService {


In Stark-Core we have an interface beside the the service implementation everytime.
Could you please do the same here ?

SuperITMan · 2018-08-29T12:16:28Z

packages/stark-ui/src/modules/app-sidebar/services/app-sidebar.service.ts

+	/**
+	 * Subject emitting sidebar close events
+	 */
+


Normally the comments should be glued to the variable/method it describes

SuperITMan

Could you please also provide a mock implementation of StarkAppSidebarService, as we have in stark-core ?

SuperITMan · 2018-09-03T09:04:28Z

packages/stark-ui/src/modules/app-sidebar/services/app-sidebar.service.spec.ts

+import { StarkAppSidebarOpenEvent } from "./app-sidebar-open-event.intf";
+import createSpy = jasmine.createSpy;
+
+describe("ValueService", () => {


Small issue here, it should be "AppSidebarService" instead of "ValueService"

SuperITMan · 2018-09-03T09:18:14Z

packages/stark-ui/src/modules/app-sidebar/services/app-sidebar.service.intf.ts

+import { Observable } from "rxjs";
+import { StarkAppSidebarOpenEvent } from "./app-sidebar-open-event.intf";
+
+/**


Could you please provide a name and an injection token as we have in stark-core ?

/** * The name of the service in case an injection is needed */ export const starkAppSidebarServiceName: string = "StarkAppSidebarService"; /** * The InjectionToken version of the service name */ export const STARK_APP_SIDEBAR_SERVICE: InjectionToken<StarkAppSidebarService> = new InjectionToken<StarkAppSidebarService>(starkAppSidebarServiceName);

SuperITMan · 2018-09-03T09:18:39Z

packages/stark-ui/src/modules/app-sidebar/services/app-sidebar.service.intf.ts

+ * Service to fetch the user profile from the REST API.
+ * In Development, it can also be used to set the user profile manually.
+ */
+export interface StarkAppSidebarServiceIntf {


Could you please change the name of the interface to StarkAppSidebarService ?

SuperITMan · 2018-09-03T09:19:11Z

packages/stark-ui/src/modules/app-sidebar/services/app-sidebar.service.ts

+ * StarkAppSidebarService service
+ */
+@Injectable()
+export class StarkAppSidebarService implements StarkAppSidebarServiceIntf {


Could you please change the name of the class to StarkAppSidebarServiceImpl

Could you please add a constructor and inject @Inject(STARK_LOGGING_SERVICE) public logger: StarkLoggingService in it ?
And then, do this inside:

public constructor(@Inject(STARK_LOGGING_SERVICE) public logger) { this.logger.debug(starkAppSidebarServiceName + " loaded"); }

SuperITMan · 2018-09-03T09:31:09Z

packages/stark-ui/src/modules/app-sidebar/app-sidebar.module.ts

+	declarations: [StarkAppSidebarComponent],
+	imports: [CommonModule, MatSidenavModule],
+	exports: [StarkAppSidebarComponent],
+	providers: [StarkAppSidebarService]


Could you please adapt the declaration of the module like this:

@NgModule({ declarations: [StarkAppSidebarComponent], imports: [CommonModule, MatSidenavModule], exports: [StarkAppSidebarComponent] }) export class StarkAppSidebarModule { /** * Instantiates the services only once since they should be singletons * so the forRoot() should be called only by the AppModule * @link https://angular.io/guide/singleton-services#forroot * @returns a module with providers */ public static forRoot(): ModuleWithProviders { return { ngModule: StarkAppSidebarModule, providers: [{ provide: STARK_APP_SIDEBAR_SERVICE, useClass: StarkAppSidebarServiceImpl }] }; } /** * Prevents this module from being re-imported * @link https://angular.io/guide/singleton-services#prevent-reimport-of-the-coremodule * @param parentModule - the parent module */ public constructor( @Optional() @SkipSelf() parentModule: StarkAppSidebarModule ) { if (parentModule) { throw new Error("StarkAppSidebarModule is already loaded. Import it in the AppModule only"); } } }

SuperITMan · 2018-09-03T09:33:08Z

showcase/src/app/app.module.ts

@@ -187,6 +190,7 @@ export const metaReducers: MetaReducer<State>[] = ENV !== "production" ? [logger
 	providers: [
 		environment.ENV_PROVIDERS,
 		APP_PROVIDERS,
+		StarkAppSidebarService,


Could you please remove this line since it won't be used as this anymore ? 😊

SuperITMan · 2018-09-03T09:33:27Z

showcase/src/app/app.module.ts

@@ -177,6 +179,7 @@ export const metaReducers: MetaReducer<State>[] = ENV !== "production" ? [logger
 		NewsModule,
 		StarkAppLogoModule,
 		StarkAppLogoutModule,
+		StarkAppSidebarModule,


Could you please adapt this to StarkAppSidebarModule.forRoot() ?

SuperITMan · 2018-09-03T09:45:24Z

showcase/src/app/demo/sidebar/demo-sidebar.component.html

@@ -0,0 +1,3 @@
+<example-viewer [extensions]="['HTML']" filesPath="sidebar/regular" exampleTitle="SHOWCASE.DEMO.SIDEBAR.TITLE">
+	<p translate>SHOWCASE.DEMO.SIDEBAR.INTRO</p>


Could you please provide an example with calls to the sidebarService ?
You could provide 3 examples, one for each sidebar.

christophercr

Some remarks... and I think the PR should be rebased...

christophercr · 2018-09-03T15:03:00Z

packages/stark-ui/src/modules.ts

@@ -2,6 +2,8 @@ export * from "./modules/action-bar";
 export * from "./modules/app-logo";
 export * from "./modules/app-logout";
 export * from "./modules/breadcrumb";
+export * from "./modules/app-sidebar";
+export * from "./modules/keyboard-directives";


This line is duplicated: "keyboard-directives"

christophercr · 2018-09-03T15:17:17Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.ts

+	 * Mode for the left sidebar: either the menu is shown or the regular sidebar
+	 */
+	@Input()
+	public sidenavLeftMode: "regular" | "menu";


Can you create a custom type for these? That way you can re-use them in the StarkAppSidebarOpenEvent interface ;)

christophercr · 2018-09-03T15:20:15Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.ts

+	 * Component lifecycle OnDestroy hook
+	 * Prevent memory leak when component destroyed
+	 */
+	public ngOnDestroy(): void {


As a convention we put the Angular lifecycle hooks right after the constructor method and in chronological order, this means that the ngOnInit is placed before the ngOnDestriy. This way is easier to find them ;)

christophercr · 2018-09-03T15:23:09Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.html

+			<ng-content select="[stark-app-sidenav-menu]"></ng-content>
+		</ng-container>
+	</mat-sidenav>
+	<mat-sidenav #appSidenavRight class="stark-app-sidenav-right" closed mode="over" position="end" [fixedInViewport]="true"


What is the attribute "closed" used for? Is it really needed here? Shouldn't we close/open the sidebar programatically instead?

christophercr · 2018-09-03T15:27:18Z

showcase/src/app/app.component.ts

@@ -19,12 +20,35 @@ import { STARK_LOGGING_SERVICE, StarkLoggingService } from "@nationalbankbelgium
 export class AppComponent implements OnInit {
 	public appState: AppState;

-	public constructor(appState: AppState, @Inject(STARK_LOGGING_SERVICE) public logger: StarkLoggingService) {
+	@ViewChild("appSideBar")
+	public appSidebar: StarkAppSidebarComponent;


It seems this variable is never used... or am I missing something?

christophercr · 2018-09-03T15:31:59Z

packages/stark-ui/src/modules/app-sidebar/services/app-sidebar.service.spec.ts

+			closeSidebars();
+		});
+		service.close();
+		expect(closeSidebars).toHaveBeenCalled();


You can use the "toHaveBeenCalledTimes(1)" matcher to be more explicit and to be sure it was called only once

christophercr · 2018-09-03T15:40:29Z

packages/stark-ui/src/modules/app-sidebar/components/app-sidebar.component.html

@@ -0,0 +1,20 @@
+<mat-sidenav-container #appSidenavContainer>
+	<mat-sidenav #appSidenavLeft class="stark-app-sidenav-left" [ngClass]="{'stark-app-sidenav-menu': sidenavLeftMode==='menu'}" closed mode="over" [fixedInViewport]="true"


What is the attribute "closed" used for? Is it really needed here? Shouldn't we close/open the sidebar programatically instead?

christophercr · 2018-09-03T15:41:59Z

showcase/src/app/app.component.html

 			</a>
 			<a mat-list-item uiSref="stark-header" uiSrefActive="active">
 				<span matLine>Stark header</span>
 			</a>
+			<a mat-list-item uiSref="demo-table" uiSrefActive="active">


Can you move this item right after the demo-sidebar item? We should keep them in alphabetical order...

catlabs · 2018-09-05T08:25:00Z

Repushed and rebased

catlabs · 2018-09-05T14:18:41Z

I also change 2 things on the layout based on both Angular Material and Material design showcases:

header take now the full width on desktop
the main content has a x padding of 70px on smaller screen, on big screen the max width of the content is 940px

To me, the look and feel is way better with those adaptation, especially with paragraphs :-)

christophercr

Small remarks

christophercr · 2018-09-05T15:07:14Z

packages/stark-ui/src/modules/app-sidebar/testing/app-sidebar.mock.ts

+
+	public openSidebar$: Observable<StarkAppSidebarOpenEvent> = this.openSidebarSource.asObservable();
+
+	private closeSidebarSource: Subject<void> = new Subject<void>();


This property should also be "protected" right?

christophercr · 2018-09-06T10:57:52Z

packages/stark-ui/src/modules/app-sidebar/components/_app-sidebar.component.scss

+  }
+
+  .stark-app-sidenav-left {
+    width: 150px;


Isn't 150px too small for the left sidebar? Could we put at least 200px as we do for the menu?

christophercr · 2018-09-06T11:00:41Z

showcase/src/app/demo/sidebar/demo-sidebar.component.html

@@ -0,0 +1,5 @@
+<example-viewer [extensions]="['HTML', 'TS']" filesPath="sidebar/regular" exampleTitle="SHOWCASE.DEMO.SIDEBAR.TITLE">
+	<button color="primary" mat-raised-button mat-button (click)="openMenu()" translate>{{ 'SHOWCASE.DEMO.SIDEBAR.OPEN_MENU' | translate }}</button>


The "translate" feature is used twice: as a directive and as a pipe. You can simply use the directive ;)

christophercr · 2018-09-06T11:01:15Z

showcase/src/app/demo/sidebar/demo-sidebar.component.html

@@ -0,0 +1,5 @@
+<example-viewer [extensions]="['HTML', 'TS']" filesPath="sidebar/regular" exampleTitle="SHOWCASE.DEMO.SIDEBAR.TITLE">
+	<button color="primary" mat-raised-button mat-button (click)="openMenu()" translate>{{ 'SHOWCASE.DEMO.SIDEBAR.OPEN_MENU' | translate }}</button>
+	<button color="primary" mat-raised-button mat-button (click)="openLeftSidebar()" translate>{{ 'SHOWCASE.DEMO.SIDEBAR.OPEN_LEFT' | translate }}</button>


The "translate" feature is used twice: as a directive and as a pipe. You can simply use the directive ;)

christophercr · 2018-09-06T11:01:20Z

showcase/src/app/demo/sidebar/demo-sidebar.component.html

+<example-viewer [extensions]="['HTML', 'TS']" filesPath="sidebar/regular" exampleTitle="SHOWCASE.DEMO.SIDEBAR.TITLE">
+	<button color="primary" mat-raised-button mat-button (click)="openMenu()" translate>{{ 'SHOWCASE.DEMO.SIDEBAR.OPEN_MENU' | translate }}</button>
+	<button color="primary" mat-raised-button mat-button (click)="openLeftSidebar()" translate>{{ 'SHOWCASE.DEMO.SIDEBAR.OPEN_LEFT' | translate }}</button>
+	<button color="primary" mat-raised-button mat-button (click)="openRightSidebar()" translate>{{ 'SHOWCASE.DEMO.SIDEBAR.OPEN_RIGHT' | translate }}</button>


The "translate" feature is used twice: as a directive and as a pipe. You can simply use the directive ;)

ISSUES CLOSED: NationalBankBelgium#592

christophercr · 2018-09-06T15:36:11Z

@SuperITMan for your info, I've reviewed this PR and everything seems OK now so I will merge it. This way I can release a new version today so the showcase will look better now ;)

In any case, if you find any issue after reviewing this again, we can create another issue to track it ok?

catlabs added must type: feature comp: stark-ui labels Aug 27, 2018

catlabs added this to the 10.0.0-alpha.5 milestone Aug 27, 2018

catlabs self-assigned this Aug 27, 2018

catlabs requested review from SuperITMan and RobbyDeLaet August 27, 2018 09:58

catlabs force-pushed the feature/sidebar branch 2 times, most recently from 9645883 to ea077f5 Compare August 29, 2018 07:15

SuperITMan requested changes Aug 29, 2018

View reviewed changes

catlabs force-pushed the feature/sidebar branch from ea077f5 to 8d01cec Compare August 29, 2018 14:56

SuperITMan requested changes Sep 3, 2018

View reviewed changes

SuperITMan requested a review from christophercr September 3, 2018 09:47

christophercr suggested changes Sep 3, 2018

View reviewed changes

catlabs force-pushed the feature/sidebar branch 3 times, most recently from 17b7382 to 6d4e72e Compare September 5, 2018 08:24

catlabs force-pushed the feature/sidebar branch from 6d4e72e to ec279f0 Compare September 5, 2018 14:18

christophercr reviewed Sep 6, 2018

View reviewed changes

catlabs force-pushed the feature/sidebar branch 3 times, most recently from ce5fe2c to 4a474c4 Compare September 6, 2018 14:51

feat(stark-ui): implementation of the app sidebar

09eeeea

ISSUES CLOSED: NationalBankBelgium#592

catlabs force-pushed the feature/sidebar branch from 4a474c4 to 09eeeea Compare September 6, 2018 15:10

christophercr approved these changes Sep 6, 2018

View reviewed changes

christophercr merged commit ac4b76e into NationalBankBelgium:master Sep 6, 2018

This was referenced Sep 6, 2018

Showcase: Sidebar menu is not displayed correctly #569

Closed

fix(stark-demo): add AppLogo back in the header after being accidentally removed #663

Merged

catlabs deleted the feature/sidebar branch September 19, 2018 08:28

		@@ -0,0 +1,3 @@
		<example-viewer [extensions]="['HTML']" filesPath="sidebar/regular" exampleTitle="SHOWCASE.DEMO.SIDEBAR.TITLE">
		<p translate>SHOWCASE.DEMO.SIDEBAR.INTRO</p>

		@@ -0,0 +1,20 @@
		<mat-sidenav-container #appSidenavContainer>
		<mat-sidenav #appSidenavLeft class="stark-app-sidenav-left" [ngClass]="{'stark-app-sidenav-menu': sidenavLeftMode==='menu'}" closed mode="over" [fixedInViewport]="true"


		public openSidebar$: Observable<StarkAppSidebarOpenEvent> = this.openSidebarSource.asObservable();

		private closeSidebarSource: Subject<void> = new Subject<void>();

		@@ -0,0 +1,5 @@
		<example-viewer [extensions]="['HTML', 'TS']" filesPath="sidebar/regular" exampleTitle="SHOWCASE.DEMO.SIDEBAR.TITLE">
		<button color="primary" mat-raised-button mat-button (click)="openMenu()" translate>{{ 'SHOWCASE.DEMO.SIDEBAR.OPEN_MENU' \| translate }}</button>

feat(stark-ui): implementation of the app sidebar #638

feat(stark-ui): implementation of the app sidebar #638

Conversation

catlabs commented Aug 27, 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?

coveralls commented Aug 27, 2018 • edited Loading

SuperITMan commented Aug 28, 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

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

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

catlabs commented Sep 5, 2018

catlabs commented Sep 5, 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

christophercr commented Sep 6, 2018

catlabs commented Aug 27, 2018 •

edited by christophercr

Loading

coveralls commented Aug 27, 2018 •

edited

Loading