feat(stark-ui): implementation of minimap component #761

Mallikki · 2018-10-09T12:10:06Z

ISSUES CLOSED: #758

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 new behavior?

Implementation of the minimap component

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

christophercr

Some remarks... and could you also add the corresponding demo page for this component in the showcase?

christophercr · 2018-10-09T13:04:48Z

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

+<div class="stark-minimap">
+	<div class="stark-minimap-dots">
+		<ul>
+			<li *ngFor="let item of items; trackBy: $index"


Currently, Travis is failing because of this: $index.... I think you should put a function here instead

christophercr · 2018-10-09T13:05:43Z

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

+				(click)="toggleDropdownMenu()" 
+				aria-label="Toggle columns"
+				matTooltip="'STARK.TABLE.TOGGLE_COLUMNS'"
+				[matTooltipPosition]="below"


Currently, Travis is failing because of this line. I think it should be like this: matTooltipPosition="below" (without square brackets)

christophercr · 2018-10-09T13:12:02Z

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

+		</button>
+		<div class="stark-minimap-dropdown-toggle-menu">
+			<div class="stark-minimap-dropdown">
+				<mat-checkbox *ngFor="let item of items; trackBy : $index"


Currently, Travis is failing because of this: $index.... I think you should put a function here instead

christophercr · 2018-10-09T13:15:32Z

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

+ * Component to display a minimap which permits to toggle the visibility of passed elements.
+ * The minimap shows the label of the elements to display with a checkbox to enable/disable the visibility:
+ *
+ * @param string -mode Desired layout or flavour:


Now we put all the JSDocs in the property they belong to... so this one should be at the mode property

christophercr · 2018-10-09T13:15:58Z

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

+ *    - compact: displayed in a compact form. There is only a button to trigger the dropdown.
+ *    - default: basic implementation with everything
+ *
+ * @param StarkMinimapItemProperties[] - items Array of StarkMinimapItemProperties objects which define the items to display in the minimap.


Now we put all the JSDocs in the property they belong to... so this one should be at the items property

christophercr · 2018-10-09T13:27:13Z

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

+
+	/**
+	 * triggers the show/hide event on an item
+	 * @param StarkMinimapItemProperties : item - the item to show/hide


I think this JSDoc should be like this: @param item - The item to show/hide... because the type is already implicit in the code ;)

christophercr · 2018-10-09T13:30:59Z

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

@@ -0,0 +1,32 @@
+<div class="stark-minimap">


Now it is not necessary to add the "stark-xxxx" class anymore... we should do this via Angular by adding this to the component class:

@Component({ selector: "stark-minimap", templateUrl: "./minimap.component.html", encapsulation: ViewEncapsulation.None, host: { class: componentName } }) export class StarkMinimapComponent implements OnInit, OnChanges, OnDestroy { ...

You can even remove this div at the end.

By doing all this, you might need to adapt the CSS rules as well since the hierarchy of HTML elements changed

christophercr · 2018-10-09T13:35:48Z

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

+	templateUrl: "./minimap.component.html",
+	encapsulation: ViewEncapsulation.None
+})
+export class StarkMinimapComponent implements OnInit, OnChanges, OnDestroy {


Now, every UI component in Stark-UI should extend the AbstractStarkUiComponent class, which is generic way to add the color input to all our components to make them themable ;)

You can have a look at the DropdownComponent how is this done

christophercr · 2018-10-09T13:37:42Z

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

+				);
+
+				if (!parentElement) {
+					this.ngZoneService.run(() => this.toggleDropdownMenu());


I don't think we need to use the ngZoneService itself, perhaps we can achieve the same by using the ApplicationRef instead inside the toggleDropdownMenu() method

christophercr · 2018-10-09T13:41:21Z

packages/stark-ui/src/modules/minimap/components/_minimap.component.scss

+      display: inline-block;
+      width: 4px;
+      height: 4px;
+      background-color: mat-color($primary-dark-text-color);


Can we move all the background-color rules and also those using the mat-color(...) to a separate _minimap-theme.scss file? So those colors can be customized to make the minimap themable

Mallikki · 2018-10-16T13:57:59Z

Still need to fix the mysterious do-not-want-to-uncheck-the-box issue with @christophercr ;)

coveralls · 2018-10-18T14:45:08Z

Coverage increased (+1.9%) to 93.28% when pulling 0a0d8a5 on Mallikki:feature/stark-ui-minimap into 290934c on NationalBankBelgium:master.

christophercr · 2018-10-19T07:51:45Z

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

+		<button mat-button class="mat-icon-button stark-minimap-dropdown-toggle-button"
+				(click)="toggleDropdownMenu()"
+				aria-label="Toggle columns"
+				[matTooltip]="'STARK.TABLE.TOGGLE_COLUMNS' | translate">


No [matTooltipPosition]="below" anymore?

christophercr · 2018-10-19T07:52:12Z

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

+			<button mat-button class="mat-icon-button stark-minimap-dropdown-toggle-button"
+					(click)="toggleDropdownMenu()"
+					aria-label="Toggle columns"
+					[matTooltip]="'STARK.TABLE.TOGGLE_COLUMNS' | translate">


No [matTooltipPosition]="below" anymore?

christophercr · 2018-10-19T07:54:22Z

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

+
+/**
+ * Component to display a minimap which permits to toggle the visibility of passed elements.
+ * The minimap shows the label of the elements to display with a checkbox to enable/disable the visibility:


Typo at the end of the line ":" -> "."

Can you also remove the next empty line?

christophercr · 2018-10-19T07:57:08Z

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

+	public mode?: StarkMinimapComponentMode;
+
+	/**
+	 * EventEmitter to be called when call showHideItem callback.


Let's change this confusing description... what about: Output that will emit the selected element to be shown/hidden

christophercr · 2018-10-19T08:05:27Z

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

@@ -0,0 +1,53 @@
+<section *ngIf="mode === 'compact'; else classic_minimap" class="custom-minimap">


It is better if we use different classes for the "compact" mode and for the "classic" one... instead of using the same custom-minimap... that way it will be easier to style them separately.

The reason is that we might separate the templates as well (once we investigate a bit deeper on how to do this in Angular 5)+ 😉

christophercr · 2018-10-19T08:07:15Z

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

+	selector: "stark-minimap",
+	templateUrl: "./minimap.component.html",
+	encapsulation: ViewEncapsulation.None
+})


Can you add the host property in here to add the stark-minimap class to the component?

host: { class: componentName }

We must do this for every component in Stark UI ;)

christophercr · 2018-10-19T08:08:36Z

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

+	}
+
+	public ngOnInit(): void {
+		this.isDisplayedMenu = false;


Don't forget to call super.ngOnInit() inside this ngOnInit so that the logic from the abstract component is also executed

ISSUES CLOSED: #758t pu

Mallikki · 2018-11-05T11:02:34Z

Will reopen with another branch, the changes are the same, I've messed up, sorry :/

christophercr · 2018-11-05T16:17:07Z

Re-created in #824

Mallikki requested a review from christophercr October 9, 2018 12:10

Mallikki force-pushed the feature/stark-ui-minimap branch 2 times, most recently from e3ad003 to a95b540 Compare October 9, 2018 12:54

christophercr suggested changes Oct 9, 2018

View reviewed changes

Mallikki force-pushed the feature/stark-ui-minimap branch 2 times, most recently from 22a3322 to 14fc6d1 Compare October 16, 2018 13:56

Mallikki force-pushed the feature/stark-ui-minimap branch 3 times, most recently from 6c6c099 to ff063f5 Compare October 18, 2018 14:18

christophercr suggested changes Oct 23, 2018

View reviewed changes

Mallikki force-pushed the feature/stark-ui-minimap branch from ff063f5 to 0a0d8a5 Compare October 29, 2018 08:16

feat(stark-ui): implementation of minimap component

7875201

ISSUES CLOSED: #758t pu

Mallikki force-pushed the feature/stark-ui-minimap branch from 0a0d8a5 to 7875201 Compare November 5, 2018 07:25

Mallikki closed this Nov 5, 2018

Mallikki deleted the feature/stark-ui-minimap branch December 11, 2018 11:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(stark-ui): implementation of minimap component #761

feat(stark-ui): implementation of minimap component #761

Mallikki commented Oct 9, 2018

christophercr left a comment

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

christophercr Oct 9, 2018

Mallikki commented Oct 16, 2018

coveralls commented Oct 18, 2018 •

edited

Loading

christophercr Oct 19, 2018

christophercr Oct 19, 2018

christophercr Oct 19, 2018

christophercr Oct 19, 2018

christophercr Oct 19, 2018

christophercr Oct 19, 2018

christophercr Oct 19, 2018

Mallikki commented Nov 5, 2018

christophercr commented Nov 5, 2018

		@@ -0,0 +1,53 @@
		<section *ngIf="mode === 'compact'; else classic_minimap" class="custom-minimap">

feat(stark-ui): implementation of minimap component #761

feat(stark-ui): implementation of minimap component #761

Conversation

Mallikki commented Oct 9, 2018

PR Checklist

PR Type

What is the new behavior?

Does this PR introduce a breaking change?

Other information

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

Mallikki commented Oct 16, 2018

coveralls commented Oct 18, 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

Mallikki commented Nov 5, 2018

christophercr commented Nov 5, 2018

coveralls commented Oct 18, 2018 •

edited

Loading