feat: (Platform) Form Group enhancement phase 1 #4213

genereg1 · 2020-12-29T20:45:30Z

Please provide a link to the associated issue.

#4076

Please provide a brief summary of this pull request.

Implement Form Grid Styles into Form Group component
Form Field column binding
Group Headers

Please check whether the PR fulfills the following requirements

the commit message follows the guideline:
https://github.com/SAP/fundamental-ngx/blob/main/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-29T20:53:39Z

✔️ Deploy preview for fundamental-ngx ready!

🔨 Explore the source changes: d7677c5

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

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

JKMarkowski · 2021-01-11T11:13:32Z

libs/platform/src/lib/components/form/form-field.ts

@@ -35,6 +35,9 @@ export abstract class FormField {
     * Indicates in which FormZone a form field should be rendered
     */
    zone: FormZone;
+
+
+    column: number; // temp type


Could you add describing comment there?

libs/platform/src/lib/components/form/form-group/form-field-group/form-field-group.component.ts

libs/platform/src/lib/components/form/form-group/form-field/form-field.component.html

libs/platform/src/lib/components/form/form-group/form-field-group/form-field-group.component.ts

libs/platform/src/lib/components/form/form-group/form-field/form-field.component.ts

libs/platform/src/lib/components/form/form-group/form-group.component.ts

dimamarksman

Please see my comments

apps/docs/src/assets/NEW_COMPONENT.md

apps/docs/src/assets/README.md

libs/platform/src/lib/components/form/form-group.ts

libs/platform/src/lib/components/form/form-group/form-field-group/form-field-group.component.ts

libs/platform/src/lib/components/form/form-group/form-field/form-field.component.ts

libs/platform/src/lib/components/form/form-group/form-group.component.ts

fkolar

Allot was already commented also by dima, let's address all these changes and do another iteration

libs/platform/src/lib/components/form/form-group.ts

fkolar · 2021-01-12T11:41:36Z

libs/platform/src/lib/components/form/form-field.ts

@@ -35,6 +35,10 @@ export abstract class FormField {
     * Indicates in which FormZone a form field should be rendered
     */
    zone: FormZone;


Do we need FormZone? Do we even have zones In new design?
The same goes to fluid. do we have such functionality now ?

Not need more. Removed

fkolar · 2021-01-12T11:42:34Z

libs/platform/src/lib/components/form/form-group/form-field/form-field.component.html

@@ -3,20 +3,22 @@
 </ng-template>


I dont think we need this section anymore.. forceRender

fkolar · 2021-01-12T11:45:48Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

 import { Subject } from 'rxjs';
 import { coerceBooleanProperty } from '@angular/cdk/coercion';

 import { FormField } from '../form-field';
 import { FormGroupContainer } from '../form-group';
-import { LabelLayout, HintPlacement, FormZone } from '../form-options';
+import { FormZone, HintPlacement, LabelLayout } from '../form-options';


As I already commented in form-field.ts do we need formZones. Is this in new Design? We have columns instead no?

libs/platform/src/lib/components/form/form-group.ts

libs/platform/src/lib/components/form/form-field.ts

fkolar · 2021-01-12T11:49:36Z

libs/platform/src/lib/components/form/form-group/form-group.component.html

+                <h1 class="fd-form-group__header-text">{{ row.value.label }}</h1>
+            </div>
+
+            <ng-container *ngFor="let r of (row.value.label ? row.value.fields: row.value) | keyvalue; trackBy: trackByFieldName">


We should maintain existing functionality for nested.

But FormGroup has FormFields or FormFieldsGroup
FormFieldGroup supports fields.

I would not complicate this now,

fkolar · 2021-01-12T11:51:38Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

-        public name: string,
-        public rank: number,
+        public name?: string,
+        public rank?: number,
        public renderer?: TemplateRef<any>,
        public columns: number = 6,
        public isFluid: boolean = false,


Do we still need this fluid?

fkolar · 2021-01-12T11:52:58Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

            }
        }
-        return;
    }
 }

 export class GroupField {


GroupField was that as internal structure to keep fields with some additional metadata used for rendering. How our FiledGroup fits (the heading we have)

Children of FormFieldGroup wrapped GroupField as well. The heading stored separately

fkolar · 2021-01-12T11:58:50Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

-            for (let i = 0; i <= Math.abs(zLeft.length - zRight.length); i++) {
-                const zone = toEven[0].zone;
-                toEven.push(new GroupField(zone, `${zone}-${i}`, toEven.length + 1, null, 6));
+    private _setUserSpecifiedLayout(groupField?: GroupField): void {


We need to also validate highest column numbers with our layout pattern.

So will not have XL2, but you will try to place column into number 4.

I don't think so we need to validate this case. At now if a form has 2 columns and a field will try bind to the 4th column, it will set at the last available column

libs/platform/src/lib/components/form/form-group/form-group.component.ts

fkolar · 2021-01-13T16:42:51Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

-                    f.styleClass = `col-sm-${f.columns} col-md-${f.columns} col-lg-${f.columns}`;
-                    merged[current++] = f;
-                    indexR++;
+    private _setUserLayout(): void {


Where do you have the validation? that your columnLayout is using smaller numbers than your column? We should also throw exception in dev mode

Added separate method

fkolar · 2021-01-13T16:49:02Z

libs/platform/src/lib/components/form/form-group/form-field/form-field.component.ts

@@ -294,9 +283,13 @@ export class FormFieldComponent implements FormField, AfterContentInit, AfterVie
     * Add FormField to FormGroup
     */
    private addToFormGroup(): void {
-        if (!this.formGroupContainer) {
+        // prevent adding wrapped fields into field group into form group container
+        const isFieldGroup = this._elementRef.nativeElement.parentElement.tagName.toLowerCase() === 'fdp-form-field-group';


When we are using this long dotted path, can some of this be NULL?
This might not with metaUI (the rule engine)

where we wrap these things with extra tag so it coudl like like:

<from-group> <meta-ui-tag-that generantes bellow code> <form-group-field> <meta-ui-tag-that generantes bellow code> ` <form-field>

So I would try to lookup closest element instead direct parent

libs/platform/src/lib/components/form/form-group/form-field/form-field.component.ts

libs/platform/src/lib/components/form/form-group.ts

JKMarkowski

All my comments have been resolved

valorkin

lgtm to me in general, but there are several minor comments

please drop empty file apps/docs/src/app/platform/component-docs/platform-forms/platform-form-container/platform-form-container-header/platform-form-container-header.component.scss

valorkin · 2021-01-18T09:13:27Z

...s/platform-forms/checkbox/platform-checkbox-examples/platform-binary-checkbox.component.html

@@ -1,16 +1,16 @@
 <h3>Binary Checkbox in Template Driven Form</h3>
 <fdp-form-group [multiLayout]="true" [noLabelLayout]="false">
-    <fdp-form-field [id]="'yellow'" [label]="'Yellow'" [noLabelLayout]="true" zone="zLeft" rank="4" [editable]="true">
+    <fdp-form-field [id]="'yellow'" [label]="'Yellow'" [noLabelLayout]="true" rank="4" [editable]="true">


please don't use unnecessary [] bindings and if you see one please remove them (could be done via regexp replace)
it's not only hurts performance for no good reason, but additionally consumers of our lib should not do it too

relative to all modified files, please ;)

valorkin · 2021-01-18T09:20:58Z

...s/platform-forms/platform-input/platform-input-example/platform-input-example.component.html

            rank="1"
-            [placeholder]="'Field placeholder text'"
+            placeholder="Field placeholder text"


e2e/wdio/platform/pages/combobox.po.ts

valorkin · 2021-01-18T09:29:01Z

libs/platform/src/lib/components/form/form-group/form-group.component.html

+                    <div class="fd-form-group fd-col__form-group fd-col" [ngClass]="xlCol">
+                        <div class="fd-form-item fd-row__form-item fd-row">
+                            <div
+                                *ngFor="let f of r.value; trackBy: trackByFieldName"


f or r.value, no idea what this means. please use a bit more descriptive names for variables

valorkin · 2021-01-18T09:31:25Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

-                    if (!this.bZone) {
-                        this.bZone = [];
+    private _updateFieldByColumn(): void {
+        const rows: { [key: number]: FieldColumn } | { [key: number]: FieldGroup } = {};


should not it will be easier? or this doesn't work?

Suggested change

const rows: { [key: number]: FieldColumn } | { [key: number]: FieldGroup } = {};

const rows: { [key: number]: FieldColumn | FieldGroup } = {};

libs/platform/src/lib/components/form/form-group/form-group.component.ts

valorkin · 2021-01-18T09:36:56Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

+
+        for (const child of this.formChildren) {
+            if (this._isFieldChild(child)) {
+                const f = this._getField(child);


f? please use more readable variable names

libs/platform/src/lib/components/form/form-group/form-group.component.ts

valorkin · 2021-01-18T09:40:25Z

libs/platform/src/lib/components/form/form-group/form-group.component.ts

+            this._getColumnNumbers();
+            if (isNaN(this.xlColumnsNumber) || isNaN(this.lgColumnsNumber) || isNaN(this.mdColumnsNumber)) {
+                throw new Error('Input a valid number for columns');
+            } else if (this.xlColumnsNumber > 12 || this.lgColumnsNumber > 12 || this.mdColumnsNumber > 12) {


no need for else if when you have throw or return in if block

valorkin

lgtm

…ies in components

kavya-b mentioned this pull request Jan 1, 2021

Integrate form grid to Platform Forms #3806

Closed

genereg1 self-assigned this Jan 5, 2021

genereg1 changed the title ~~[WIP] feat: (platform) Form Group enhancement phase 1~~ feat: (platform) Form Group enhancement phase 1 Jan 6, 2021

genereg1 added the platform platform label Jan 6, 2021

genereg1 changed the title ~~feat: (platform) Form Group enhancement phase 1~~ feat: (Platform) Form Group enhancement phase 1 Jan 6, 2021

genereg1 requested a review from a team January 11, 2021 09:06

JKMarkowski reviewed Jan 11, 2021

View reviewed changes

JKMarkowski suggested changes Jan 11, 2021

View reviewed changes

genereg1 requested review from JKMarkowski and fkolar January 12, 2021 09:21

dimamarksman suggested changes Jan 12, 2021

View reviewed changes

fkolar requested changes Jan 12, 2021

View reviewed changes

genereg1 requested review from fkolar and dimamarksman January 13, 2021 15:53

fkolar requested changes Jan 13, 2021

View reviewed changes

dimamarksman reviewed Jan 13, 2021

View reviewed changes

libs/platform/src/lib/components/form/form-group/form-field/form-field.component.ts Outdated Show resolved Hide resolved

dimamarksman suggested changes Jan 13, 2021

View reviewed changes

libs/platform/src/lib/components/form/form-group.ts Outdated Show resolved Hide resolved

genereg1 force-pushed the feat/form-enhancement-phase-1 branch from 5704095 to 829bd4e Compare January 14, 2021 08:38

genereg1 requested review from fkolar and dimamarksman January 14, 2021 08:56

JKMarkowski approved these changes Jan 14, 2021

View reviewed changes

genereg1 force-pushed the feat/form-enhancement-phase-1 branch from 829bd4e to 583ecc7 Compare January 14, 2021 13:54

manjunathanagaraj mentioned this pull request Jan 15, 2021

Sprint 54 - goals #3756

Closed

dimamarksman approved these changes Jan 15, 2021

View reviewed changes

genereg1 force-pushed the feat/form-enhancement-phase-1 branch from 247f8a4 to f13d294 Compare January 16, 2021 10:17

valorkin suggested changes Jan 18, 2021

View reviewed changes

genereg1 force-pushed the feat/form-enhancement-phase-1 branch from 5540013 to 3ab72f3 Compare January 18, 2021 13:44

valorkin approved these changes Jan 19, 2021

View reviewed changes

kavya-b and others added 23 commits January 20, 2021 15:48

feat: (platform) form grid styles for Platform Forms

fdde2e7

address review comments

03a5262

address review comments

ef7624c

easy layout

88b012e

4 column view

22893ae

add column property aligment

46e3fca

feat(platform): add field-group component

d1108a3

feat(platform): form group enhancement phase 1

0aa011f

feat(platform): rewrite column template

7845970

feat(platform): fix field group in others components

6443eb2

feat(platform): fix circular dependency

86f28af

feat(platform): add field group padding

da69ee1

feat(platform): update docs and fix PR comments

84689f2

fix(platform): fix adressed comments

55ed649

feat(platform): fix review comments

3036c88

feat(platform): change default column layout value

a56a140

feat(platform): add form helpers && address comments

22f24d5

test(e2e): update e2e test locators

07d8ae4

feat(platform): fix circular dependency

d2355d4

feat(platform): enchance fields render

356386d

feat(platform): fix travis build

6c0ea5c

feat(platform): remove multilayout property clean up leftover propert…

6f16de3

…ies in components

feat(platform): remove unused styles

e4d1bbb

genereg1 force-pushed the feat/form-enhancement-phase-1 branch from f412acf to e4d1bbb Compare January 20, 2021 13:49

Fixed failed scenarios. Small refactoring

d7677c5

genereg1 requested a review from fkolar January 20, 2021 16:31

fkolar approved these changes Jan 21, 2021

View reviewed changes

genereg1 merged commit 064955e into main Jan 21, 2021

genereg1 deleted the feat/form-enhancement-phase-1 branch January 21, 2021 08:39

manjunathanagaraj mentioned this pull request Jan 29, 2021

Sprint 55 - ariba: Summary #4524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: (Platform) Form Group enhancement phase 1 #4213

feat: (Platform) Form Group enhancement phase 1 #4213

genereg1 commented Dec 29, 2020 •

edited

netlify bot commented Dec 29, 2020 •

edited

JKMarkowski Jan 11, 2021

dimamarksman left a comment

fkolar left a comment

fkolar Jan 12, 2021

genereg1 Jan 13, 2021

fkolar Jan 12, 2021

fkolar Jan 12, 2021

genereg1 Jan 13, 2021

fkolar Jan 12, 2021

fkolar Jan 12, 2021

genereg1 Jan 13, 2021

fkolar Jan 12, 2021

genereg1 Jan 13, 2021

fkolar Jan 12, 2021

genereg1 Jan 13, 2021

fkolar Jan 13, 2021

genereg1 Jan 14, 2021

fkolar Jan 13, 2021

JKMarkowski left a comment

valorkin left a comment

valorkin Jan 18, 2021

valorkin Jan 18, 2021

valorkin Jan 18, 2021

valorkin Jan 18, 2021

valorkin Jan 18, 2021

valorkin Jan 18, 2021

valorkin Jan 18, 2021

valorkin left a comment

	const rows: { [key: number]: FieldColumn } \| { [key: number]: FieldGroup } = {};
	const rows: { [key: number]: FieldColumn \| FieldGroup } = {};

feat: (Platform) Form Group enhancement phase 1 #4213

feat: (Platform) Form Group enhancement phase 1 #4213

Conversation

genereg1 commented Dec 29, 2020 • edited

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 29, 2020 • edited

Choose a reason for hiding this comment

dimamarksman left a comment

Choose a reason for hiding this comment

fkolar 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

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

JKMarkowski left a comment

Choose a reason for hiding this comment

valorkin 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

valorkin left a comment

Choose a reason for hiding this comment

genereg1 commented Dec 29, 2020 •

edited

netlify bot commented Dec 29, 2020 •

edited