feat: (Platform) introduce Approval Flow component - Phase 1 #4059

IlyaSurmay · 2020-12-09T12:44:39Z

Please provide a link to the associated issue.

#3403
tech spec - https://app.gitbook.com/@fundamental-ngx/s/docs/platform-approval-flow

Please provide a brief summary of this pull request.

This PR introduces a new Approval Flow component (phase 1 implementation)
Functionality included in Phase 1:

render approval flow graph
show watchers
send reminder to single approver
send reminders to multiple approvers
show details of member of approval team
show details of watcher
carousel mode
keyboard navigation

Please check whether the PR fulfills the following requirements

the commit message follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/master/CONTRIBUTING.md
tests for the changes that have been done
all items on the PR Review Checklist are addressed :
https://github.com/SAP/fundamental-ngx/wiki/PR-Review-Checklist

Documentation checklist:

update README.md
Breaking Changes Wiki
Documentation Examples
Stackblitz works for all examples

netlify · 2020-12-09T12:49:56Z

✔️ Deploy preview for fundamental-ngx ready!

🔨 Explore the source changes: 0d3f21e

🔍 Inspect the deploy logs: https://app.netlify.com/sites/fundamental-ngx/deploys/5fe1d53fd7360d000791e39d

😎 Browse the preview: https://deploy-preview-4059--fundamental-ngx.netlify.app

dimamarksman

Looks good!
Added some comments.

libs/platform/src/lib/components/approval-flow/interfaces/user.ts

libs/platform/src/lib/components/approval-flow/approval-flow.module.ts

libs/platform/src/lib/components/approval-flow/approval-flow.component.ts

libs/platform/src/lib/components/approval-flow/approval-flow.component.scss

.../components/approval-flow/approval-flow-user-details/approval-flow-user-details.component.ts

...omponents/approval-flow/approval-flow-user-details/approval-flow-user-details.component.html

dimamarksman · 2020-12-10T11:17:41Z

Looks broken in RTL mode

Betrozov · 2020-12-14T13:03:51Z

...atform/src/lib/components/approval-flow/approval-flow-node/approval-flow-node.component.html

+        <div class="approval-flow-node__info">
+            <div class="approval-flow-node__name">
+                <ng-container *ngIf="node.approvers.length === 1">{{ node.approvers[0].name }}</ng-container>
+                <ng-container *ngIf="node.approvers.length > 1">{{ node.approvers.length }} members</ng-container>


please add internalization

sKudum · 2020-12-15T09:57:00Z

...omponents/approval-flow/approval-flow-user-details/approval-flow-user-details.component.scss

+                color: #0854A0;
+                color: var(--sapButton_IconColor, #0854A0);
+                position: relative;
+                top: 1px;


Can we have it rem

sKudum · 2020-12-15T09:58:10Z

...omponents/approval-flow/approval-flow-user-details/approval-flow-user-details.component.scss

+        fdp-standard-list-item {
+            .fd-checkbox__label {
+                position: relative;
+                left: 5px;


We need to try to avoid using px, it will impact responsiveness.

sKudum · 2020-12-15T10:02:22Z

libs/platform/src/lib/components/approval-flow/approval-flow.component.spec.ts

+@Component({
+    selector: 'fdp-test-approval-flow',
+    template: `
+        <fdp-approval-flow [title]="title" [dataSource]="dataSource"></fdp-approval-flow>`


Just a query, how datasource is helping here us comparing to using angular basic approaches(*ngFor, array etc)

Using dataSource was a requirement from the tech spec.

I don't get why *ngFor, array is compared to the DataSource here, they work independently, DataSource provides methods for retrieving approval flow's data and manipulating that data in any way, *ngFor and etc are internal Angular tools that are used quite a lot in this component's code.

As far as I can tell, for a complex component like this one using DataSource is preferable way, otherwise the component's number of inputs and outputs would be quite big. Also using DataSource allows to have all data logic in one place

sKudum · 2020-12-15T10:03:39Z

libs/platform/src/lib/components/approval-flow/interfaces/approval-data-source.ts

+     * Update watcher list. Called whenever there is a change
+     * to the watcher list.
+     */
+    updateWatchers(watchers: ApprovalUser[]): void;


Same query as above, how a datasource with abstract functions helping in workflow

answered above

sKudum · 2020-12-15T10:09:49Z

it seems RTL mode it is breaking.

Betrozov · 2020-12-15T13:17:45Z

is it correct?

IlyaSurmay · 2020-12-16T13:07:02Z

@Betrozov @sKudum
RTL has been fixed

@Betrozov no designs were provided for different themes so maybe need to clarify this with the designers. I've changed some variables so now it looks more or less OK with different themes

@sKudum changed px to rem, left only a few in places where rem isn't suitable such as fixed dimensions or border widths

sKudum · 2020-12-16T13:26:21Z

@Betrozov @sKudum
RTL has been fixed

@Betrozov no designs were provided for different themes so maybe need to clarify this with the designers. I've changed some variables so now it looks more or less OK with different themes

@sKudum changed px to rem, left only a few in places where rem isn't suitable such as fixed dimensions or border widths

Thank you @IlyaSurmay for addressing the comments. Could you please have a look at this. (In RTL mode content is aligned to LTR)

dimamarksman

Thank you for addressing my comments!
I left one more regarding translation.

libs/platform/src/lib/components/approval-flow/approval-flow.component.html

.../components/approval-flow/approval-flow-user-details/approval-flow-user-details.component.ts

InnaAtanasova · 2020-12-17T19:22:34Z

.../components/approval-flow/approval-flow-user-details/approval-flow-user-details.component.ts

+    }
+
+    /** @hidden */
+    setListItems(items: ApprovalUser[]): void {


all hidden methods should be prefixed with _

InnaAtanasova

Impressive! LGTM

IlyaSurmay · 2020-12-22T12:35:56Z

closed in favor of #4177

IlyaSurmay added the platform platform label Dec 9, 2020

IlyaSurmay self-assigned this Dec 9, 2020

IlyaSurmay marked this pull request as ready for review December 9, 2020 13:30

InnaAtanasova requested a review from a team December 9, 2020 13:41

InnaAtanasova added this to the Sprint 52 - Ariba milestone Dec 9, 2020

dimamarksman suggested changes Dec 10, 2020

View reviewed changes

Betrozov reviewed Dec 14, 2020

View reviewed changes

sKudum reviewed Dec 15, 2020

View reviewed changes

IlyaSurmay force-pushed the feat-approval-flow-phase-1 branch from ea80e4a to 1c80c9e Compare December 16, 2020 13:03

IlyaSurmay requested review from dimamarksman, sKudum and Betrozov December 16, 2020 13:07

dimamarksman reviewed Dec 16, 2020

View reviewed changes

libs/platform/src/lib/components/approval-flow/approval-flow.component.html Outdated Show resolved Hide resolved

dimamarksman approved these changes Dec 16, 2020

View reviewed changes

manjunathanagaraj mentioned this pull request Dec 16, 2020

Sprint 52 - goals #3732

Closed

IlyaSurmay commented Dec 17, 2020

View reviewed changes

.../components/approval-flow/approval-flow-user-details/approval-flow-user-details.component.ts Outdated Show resolved Hide resolved

IlyaSurmay force-pushed the feat-approval-flow-phase-1 branch from 194abf0 to 8844dd4 Compare December 17, 2020 17:04

InnaAtanasova reviewed Dec 17, 2020

View reviewed changes

InnaAtanasova approved these changes Dec 17, 2020

View reviewed changes

sKudum approved these changes Dec 18, 2020

View reviewed changes

JKMarkowski changed the base branch from master to main December 18, 2020 17:26

IlyaSurmay added 3 commits December 22, 2020 11:35

approval flow init

bc75598

add approval flow inputs and data types

f2548f0

approval flow node component, mock approval process class

5fdb853

IlyaSurmay added 25 commits December 22, 2020 11:35

render approval nodes tree

1792d99

approver and watcher dialogs

38195df

render members list in multiple approvers dialog

00ff843

improve graph constructor logic

5d6f82c

display connections between nodes

b1f075c

add carousel support

910cd24

add templateRef support for user details

17cf4e5

fetch user data in user details dialog

456a1e0

fix send multiple reminders

18ba99a

add search to members list

08aee8e

add keyboard navigation

a0fa3a1

add i18n marker to watchers label, align header styles with design

2933955

add tests

03f153b

clean up

9ff3ce8

clean up, fix BEM, use css variables

9cef446

fix tests

8501af1

add unsub from data source, change examples in demo

8e0f6e8

fix members search

fcc0807

member search by id

a0abf5e

remove spec file from docs

532e465

address review comments

a146b4a

fix RTL, address review comments

241c826

fix RTL in user details dialog, address review comments

036f413

add plural clause to approval flow node members

73ca20e

remove unnecessary Inject dialogRef

89b6222

IlyaSurmay force-pushed the feat-approval-flow-phase-1 branch from 8844dd4 to 89b6222 Compare December 22, 2020 09:44

add underscore to hidden methods' names

0d3f21e

IlyaSurmay mentioned this pull request Dec 22, 2020

feat: (Platform) introduce Approval Flow component - Phase 1 #4177

Merged

7 tasks

IlyaSurmay closed this Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: (Platform) introduce Approval Flow component - Phase 1 #4059

feat: (Platform) introduce Approval Flow component - Phase 1 #4059

IlyaSurmay commented Dec 9, 2020

netlify bot commented Dec 9, 2020 •

edited

Loading

dimamarksman left a comment

dimamarksman commented Dec 10, 2020

Betrozov Dec 14, 2020

sKudum Dec 15, 2020

sKudum Dec 15, 2020

sKudum Dec 15, 2020

IlyaSurmay Dec 16, 2020

sKudum Dec 15, 2020

IlyaSurmay Dec 16, 2020

sKudum commented Dec 15, 2020

Betrozov commented Dec 15, 2020 •

edited

Loading

IlyaSurmay commented Dec 16, 2020

sKudum commented Dec 16, 2020

dimamarksman left a comment

InnaAtanasova Dec 17, 2020

InnaAtanasova left a comment

IlyaSurmay commented Dec 22, 2020

feat: (Platform) introduce Approval Flow component - Phase 1 #4059

feat: (Platform) introduce Approval Flow component - Phase 1 #4059

Conversation

IlyaSurmay commented Dec 9, 2020

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

Please check whether the PR fulfills the following requirements

netlify bot commented Dec 9, 2020 • edited Loading

dimamarksman left a comment

Choose a reason for hiding this comment

dimamarksman commented Dec 10, 2020

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

sKudum commented Dec 15, 2020

Betrozov commented Dec 15, 2020 • edited Loading

IlyaSurmay commented Dec 16, 2020

sKudum commented Dec 16, 2020

dimamarksman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InnaAtanasova left a comment

Choose a reason for hiding this comment

IlyaSurmay commented Dec 22, 2020

netlify bot commented Dec 9, 2020 •

edited

Loading

Betrozov commented Dec 15, 2020 •

edited

Loading