fix: (platform) Introduce Checkbox as a Component #2293

DeepakSap14 · 2020-04-08T13:42:22Z

Please provide a link to the associated issue.

Please provide a brief summary of this pull request.

Created Checkbox component in platform, which works with platform FormFieldControl.
It works fine with Reactive form as well as template driven form.

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

Documentation checklist:

update README.md
Documentation Examples
Stackblitz works for all examples

Technical specification:
https://github.com/SAP/fundamental-ngx/wiki/Platform:-Checkbox-Component-Technical-Design

netlify · 2020-04-08T13:45:23Z

Deploy preview for fundamental-ngx ready!

Built with commit 6bf9738

https://deploy-preview-2293--fundamental-ngx.netlify.app

fkolar · 2020-04-08T14:47:45Z

Build failed

DeepakSap14 · 2020-04-08T15:18:11Z

Build failed

Build is passed, but deploy is failing. there may be some issue with netify. will check with Deno and team.
I have also checked it in local with "npm run build-all", it was passing.

DeepakSap14 · 2020-04-09T03:51:31Z

Deployment failing is happening for other PR as well. once reviewed will check on that issue.

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

...cs/platform-forms/checkbox/platform-checkbox-examples/platform-checkbox-styling.component.ts

DeepakSap14 · 2020-04-15T06:46:07Z

Have pushed new changes based on reviews.
This is platform checkbox implementation only (not checkbox group), there will be a separate PR for checkbox group.

InnaAtanasova

I see a lot of any for types and methods are missing return type.

...cs/platform-forms/checkbox/platform-checkbox-examples/platform-checkbox-example.component.ts

.../platform-forms/checkbox/platform-checkbox-examples/platform-checkbox-styling.component.html

...cs/platform-forms/checkbox/platform-checkbox-examples/platform-checkbox-styling.component.ts

fkolar

it does not feel right, the component is not really using things from baseInput + it will not really work with formfield formgroup structure..

Try to look at the previous implementation I prepared for D-com. even its hacky, you see some structure that I put in place https://github.com/ngx-meta/rules/tree/master/libs/fiori-rules

fkolar · 2020-04-17T08:38:54Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+        return super.getValue();
+    }
+    set value(value: any) {
+        if (value) {


check parent implementation what it is doing, when setting a value, I need to emit event to parent formField so form field can call markForCheck.

there is stateChanges - please check it out.

Also what is the added value set your value here instead of setValue from parent?

fkolar · 2020-04-17T08:40:55Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+    }
+
+    /** ControlvalueAccessor */
+    writeValue(value: any) {


check what parent writeValue is doing and why

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

fkolar · 2020-04-17T08:49:48Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+    }
+
+    /** @hidden captures controller value */
+    private modelValue: any;


why do you need this extra property, why cant you set it directly? to value ?

now using set value method to call setValue() of baseInput.

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

fkolar · 2020-04-17T08:53:10Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.html

@@ -0,0 +1,14 @@
+<fd-checkbox


Also I see only checkbox here but how this is going to work with FormField and FormGroup? It needs to be part of it, so in formField you set label for the field ..

have included " div fd-form-item " in template.

droshev

What are the breaking changes?

DeepakSap14 · 2020-04-20T03:56:47Z

What are the breaking changes?

@droshev
In platform we need checkbox something like "https://primefaces.org/primeng/showcase/#/checkbox".
This will be easy to integrate with MetaUi Implementation. current implementation in checkbox expects to have values {"truevalue: '', falseValue: '', tristatevalue:''} passed. but with this implementation, application developer have to just pass value='checkboxvalueonselect', and on select it will update controller. on uncheck, it will be removed from controller.

droshev · 2020-04-20T19:03:22Z

What are the breaking changes?

@droshev
In platform we need checkbox something like "https://primefaces.org/primeng/showcase/#/checkbox".
This will be easy to integrate with MetaUi Implementation. current implementation in checkbox expects to have values {"truevalue: '', falseValue: '', tristatevalue:''} passed. but with this implementation, application developer have to just pass value='checkboxvalueonselect', and on select it will update controller. on uncheck, it will be removed from controller.

My comment was related to the changes you did with this PR. Do you introduce a breaking changes with this PR? If not then you need to update the title of the PR.

fix: nested tristate remaining, documentation done

fkolar · 2020-06-11T17:10:39Z

libs/platform/src/lib/components/form/checkbox/checkbox.component.ts

+})
+export class CheckboxComponent extends BaseInput implements AfterViewInit {
+    /** set to true if binary checkbox */
+    @Input()


I am more or less ok with this version I would work on more on the Selection Model and remove all the code and replace it with Selection Model that exists in the Material/cdk. it will make things much cleaner

stefanoScalzo · 2020-06-15T15:30:06Z

After clicking checkbox quickly the page generates errors out

stefanoScalzo

see comment

DeepakSap14 · 2020-06-16T06:35:23Z

After clicking checkbox quickly the page generates errors out

This issue is coming from core checkbox. Will raise issue for fix in core checkbox.

JKMarkowski

Code LGTM, let's take a look at 1 comment, but I guess it's not in scope of this PR.

...rm-forms/checkbox/platform-checkbox-examples/platform-checkbox-error-handling.component.html

JKMarkowski · 2020-06-16T11:48:07Z

@stefanoScalzo @DeepakSap14

After clicking checkbox quickly the page generates errors out

This issue is coming from core checkbox. Will raise issue for fix in core checkbox.

This issue was reported some time ago. It's not even ngx problem, but chrome&styles issue.

All required changes are done.

sKudum

LGTM

DeepakSap14 · 2020-06-17T04:02:20Z

Approvals are done. It looks Good for me. Merging it.

DeepakSap14 self-assigned this Apr 8, 2020

DeepakSap14 added the platform platform label Apr 8, 2020

DeepakSap14 requested review from kavya-b, Lokanathan-k, KevinOkamoto and fkolar April 9, 2020 03:50

DeepakSap14 added this to In Review in Platform Development Apr 9, 2020

DeepakSap14 linked an issue Apr 9, 2020 that may be closed by this pull request

fix: (Platform) Checkbox Component created in Platform #2292

Closed

fkolar requested changes Apr 9, 2020

View reviewed changes

DeepakSap14 force-pushed the feat/checkbox branch 2 times, most recently from 9e1683b to fc693e8 Compare April 15, 2020 06:43

DeepakSap14 requested a review from sKudum April 15, 2020 06:46

InnaAtanasova changed the title ~~feat: (Platform) Checkbox As a Component~~ feat: (platform) Checkbox As a Component Apr 16, 2020

InnaAtanasova previously requested changes Apr 16, 2020

View reviewed changes

DeepakSap14 force-pushed the feat/checkbox branch from fc693e8 to 047ed4f Compare April 16, 2020 18:54

rengare suggested changes Apr 16, 2020

View reviewed changes

fkolar requested changes Apr 17, 2020

View reviewed changes

droshev self-requested a review April 19, 2020 01:52

droshev previously requested changes Apr 19, 2020

View reviewed changes

DeepakSap14 changed the title ~~feat: (platform) Checkbox As a Component~~ [WIP] feat: (platform) Checkbox As a Component Apr 20, 2020

DeepakSap14 changed the title ~~[WIP] feat: (platform) Checkbox As a Component~~ [WIP] fix: (platform) Checkbox As a Component Apr 21, 2020

DeepakSap14 force-pushed the feat/checkbox branch from 047ed4f to 3648970 Compare April 24, 2020 04:27

DeepakSap14 requested a review from fkolar April 24, 2020 04:36

DeepakSap14 added 7 commits June 10, 2020 22:47

docs:documentation fix

f9451ed

fix:review change

f467c1d

fix: nested tristate remaining, documentation done

fix: tristate change done

7cc394e

fix:setvalue included

149eff1

docs: disabled example included

bb5252d

fix:review implementation for example changes [ci skip]

5ce1e0b

fix:multiselect example removed,value used

c8cb796

DeepakSap14 force-pushed the feat/checkbox branch from 4a8ea47 to c8cb796 Compare June 10, 2020 17:23

docs: pre tag removed from import, review

6bf9738

fkolar approved these changes Jun 11, 2020

View reviewed changes

fkolar self-requested a review June 11, 2020 17:07

fkolar reviewed Jun 11, 2020

View reviewed changes

fkolar approved these changes Jun 11, 2020

View reviewed changes

DeepakSap14 requested review from JKMarkowski and InnaAtanasova June 12, 2020 07:52

stefanoScalzo reviewed Jun 15, 2020

View reviewed changes

DeepakSap14 requested a review from stefanoScalzo June 16, 2020 06:35

JKMarkowski approved these changes Jun 16, 2020

View reviewed changes

...rm-forms/checkbox/platform-checkbox-examples/platform-checkbox-error-handling.component.html Show resolved Hide resolved

Platform Development automation moved this from In Review to Approved by Reviewer Jun 17, 2020

sKudum approved these changes Jun 17, 2020

View reviewed changes

DeepakSap14 merged commit 9f410fb into master Jun 17, 2020

DeepakSap14 deleted the feat/checkbox branch June 17, 2020 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: (platform) Introduce Checkbox as a Component #2293

fix: (platform) Introduce Checkbox as a Component #2293

DeepakSap14 commented Apr 8, 2020 •

edited

netlify bot commented Apr 8, 2020 •

edited

fkolar commented Apr 8, 2020

DeepakSap14 commented Apr 8, 2020

DeepakSap14 commented Apr 9, 2020

DeepakSap14 commented Apr 15, 2020

InnaAtanasova left a comment

fkolar left a comment

fkolar Apr 17, 2020

fkolar Apr 17, 2020

fkolar Apr 17, 2020

DeepakSap14 Jun 11, 2020

fkolar Apr 17, 2020

DeepakSap14 Jun 11, 2020 •

edited

droshev left a comment

DeepakSap14 commented Apr 20, 2020 •

edited

droshev commented Apr 20, 2020

fkolar Jun 11, 2020

stefanoScalzo commented Jun 15, 2020

stefanoScalzo left a comment

DeepakSap14 commented Jun 16, 2020

JKMarkowski left a comment

JKMarkowski commented Jun 16, 2020 •

edited

sKudum left a comment

DeepakSap14 commented Jun 17, 2020

fix: (platform) Introduce Checkbox as a Component #2293

fix: (platform) Introduce Checkbox as a Component #2293

Conversation

DeepakSap14 commented Apr 8, 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 Apr 8, 2020 • edited

fkolar commented Apr 8, 2020

DeepakSap14 commented Apr 8, 2020

DeepakSap14 commented Apr 9, 2020

DeepakSap14 commented Apr 15, 2020

InnaAtanasova left a comment

Choose a reason for hiding this comment

fkolar left a comment

Choose a reason for hiding this comment

fkolar Apr 17, 2020

Choose a reason for hiding this comment

fkolar Apr 17, 2020

Choose a reason for hiding this comment

fkolar Apr 17, 2020

Choose a reason for hiding this comment

DeepakSap14 Jun 11, 2020

Choose a reason for hiding this comment

fkolar Apr 17, 2020

Choose a reason for hiding this comment

DeepakSap14 Jun 11, 2020 • edited

Choose a reason for hiding this comment

droshev left a comment

Choose a reason for hiding this comment

DeepakSap14 commented Apr 20, 2020 • edited

droshev commented Apr 20, 2020

fkolar Jun 11, 2020

Choose a reason for hiding this comment

stefanoScalzo commented Jun 15, 2020

stefanoScalzo left a comment

Choose a reason for hiding this comment

DeepakSap14 commented Jun 16, 2020

JKMarkowski left a comment

Choose a reason for hiding this comment

JKMarkowski commented Jun 16, 2020 • edited

sKudum left a comment

Choose a reason for hiding this comment

DeepakSap14 commented Jun 17, 2020

DeepakSap14 commented Apr 8, 2020 •

edited

netlify bot commented Apr 8, 2020 •

edited

DeepakSap14 Jun 11, 2020 •

edited

DeepakSap14 commented Apr 20, 2020 •

edited

JKMarkowski commented Jun 16, 2020 •

edited