feat: (platform) Radio button group component Implementation #2122

I see you've placed the new forms code in the directory libs/platform/src/lib/components/Forms. Directory names shouldn't be capitalized. Please lower-case the directory name: libs/platform/src/lib/components/forms.

libs/platform/src/lib/components/Forms/fdp-form.module.ts

KevinOkamoto · 2020-03-05T15:46:32Z

libs/platform/src/lib/components/Forms/radio-group/radio-group.component.html

+        <fd-radio-button
+            fd-form-control
+            name="name"
+            [value]="None"


Is "None" a string value? If so, this will not work unless the string is placed in single quotes, like this:

[value]="'None'"

you dont use to use it this way. [value]="None" must even fail the compilation no? you are using binding syntax.
Also whey are u using directive fd-form-control ?

KevinOkamoto · 2020-03-05T15:48:37Z

libs/platform/src/lib/components/Forms/radio-group/radio-group.component.html

+    <div fd-form-item *ngIf="hasNoValue">
+        <fd-radio-button
+            fd-form-control
+            name="name"


Are we hard-coding "name" to the string 'name'? I think you want property binding here:

[name]="name"

KevinOkamoto · 2020-03-05T15:51:53Z

libs/platform/src/lib/components/Forms/fdp-form.module.ts

+    entryComponents: [RadioGroupComponent],
+    exports: [RadioGroupComponent]
+})
+export class FdpFormModule {}


I think we agreed to preface all our module names with Platform. Please change the name from FdpFormModule to PlatformModule

@KevinOkamoto Are you sure its Platform? I just told him its Fdp and I can pull out the thread.

Ah my bad - I guess we earlier decided on "Fdp". However all the other modules in the platform library are prefaced with "Platform", so instead of changing the names of the other modules which have already been published, let's name it "PlatformFormModule", so that we have consistent naming in our library, and from this point forward all platform modules will be prefaced with "Platform".

KevinOkamoto · 2020-03-05T16:01:48Z

I checkout your branch, and tried running the documentation app, and saw these errors in the console:

fkolar

it should not be plurar forms it should be form

libs/platform/src/lib/components/Forms/form-control.ts

libs/platform/src/lib/components/Forms/public_api.ts

fkolar · 2020-03-05T15:54:32Z

libs/platform/src/lib/components/Forms/data-model.ts

+ * custom label that is shown in the options.
+ */
+
+export type stateType = 'valid' | 'invalid' | 'warning' | 'default' | 'information';


Constant should start with capital letter

also I saw similar constant in the core, we can simply use it, we dont need to redefine it here

fkolar · 2020-03-05T15:56:21Z

libs/platform/src/lib/components/Forms/radio-group/radio-group.component.ts

+import { BaseInput } from '../base.input';
+import { SelectItem } from '../data-model';
+
+export type stateType = 'valid' | 'invalid' | 'warning' | 'default' | 'information';


Does state type applies to all of the components ? if yes then you dont want to duplicate the declaration in here. (I think I saw in some other file ). This relates to other comments I had above. Before jumping into this, let;'s think of all the common properties for Inputs.

State, Compact|Cozy -> Is there anything else ?

Similar as above for statetype. states are already defined in the core. can we use it?

fkolar · 2020-03-05T15:56:48Z

libs/platform/src/lib/components/Forms/radio-group/radio-group.component.ts

+    state: stateType = 'default';
+
+    @Input()
+    get isInline(): boolean {


You should add some documentation for this.

libs/platform/src/lib/components/Forms/radio-group/radio-group.component.ts

DeepakSap14 · 2020-03-10T03:55:42Z

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

+                    button.inputElement.nativeElement.checked = true;
+                    this._selected = button;
+                }
+                // button.onChange.Pipe(takeUntil(this.destroy$)).subscribe(ev => this._selectedRadioButtonChanged(ev));


@fkolar , Radio button have not exposed change event. By using Onchange function i am not able to get button change event. please suggest here.

DeepakSap14 · 2020-03-10T03:56:35Z

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

+    set name(value: string) {
+        if (this.name !== value) {
+            this._name = value;
+            this._updateRadioButtonNames();


@fkolar ,as content is not initialised. I am not able to update names for content Radio buttons. please suggest

fkolar · 2020-03-24T13:57:27Z

Since I was commenting on this mostly in the slack, have you addressed all the comments before I starts looking at this ? Also you might want to squash your commits. when you giving this out for a review..

DeepakSap14 · 2020-03-24T14:08:02Z

Since I was commenting on this mostly in the slack, have you addressed all the comments before I starts looking at this ? Also you might want to squash your commits. when you giving this out for a review..

i will take one more day to push the changes. i will let you know, if i push changes.
from next time i will squash the commits.

fkolar · 2020-03-29T19:26:47Z

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

@@ -0,0 +1,22 @@
+<ng-template #renderer>
+    <ng-container *ngTemplateOutlet="radio"></ng-container>


I have commented this before on slack, this is not addressed.

I was asking, why do you need this ng-template radio and extra ng-container?

fkolar · 2020-03-29T19:29:38Z

libs/platform/src/lib/components/form/radio/radio.component.spec.ts

@@ -0,0 +1,24 @@
+import { async, ComponentFixture, TestBed } from '@angular/core/testing';


Remove this file as there are not tests and issue and it will be tested as part of RB Group

Can you move Radio under RadioGroup? I know logically in case of CheckBox, it makes sense to have it as extra folder, but I dont see a usecase somebody will be using fdp-radio on its own (they could use fd-radio), but our added value is RB Group.

fkolar · 2020-03-29T19:31:01Z

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

+    changeDetection: ChangeDetectionStrategy.OnPush,
+    providers: [{ provide: FormFieldControl, useExisting: PlatformRadioButtonComponent, multi: true }]
+})
+export class PlatformRadioButtonComponent implements ControlValueAccessor {


Once again, you havent addressed thing that I said before. You asked me to comment things in slack and I have added comment for this.

Check e.g. RB implementation for radio button in material design, and comment I said in slack

fkolar · 2020-03-29T19:33:25Z

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

+export class PlatformRadioButtonComponent implements ControlValueAccessor {
+    /** name of radio button, will be set from radio button group name */
+    @Input()
+    get name(): any {


I am finishing my review as the same applies for name, its not addressed, Please come back when you address all, or at least you raise question.

I don't feel like copying over all what I said... Please next time, dont ask for input on slack..

i had not pushed changes for Radio button group. Now i have pushed.
yes slack is not a good place for comments.

sKudum

Fiori has Radio button Group, we need to maintain the same naming convention everywhere it is used.

fkolar · 2020-03-31T12:46:32Z

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

+export class RadioButtonComponent extends BaseInput {
+    /** Setting default value for mandatory field id */
+    @Input()
+    id: string = `radio-id-${uniqueId++}`;


You dont need to redefine @input for values which are in parent, if you need re-assign values, just do this in constructor. the same goes for name

fkolar · 2020-03-31T12:47:57Z

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

+
+    /** Access radio button child elemen passed as content of radio button group*/
+    @ViewChild(CoreRadioButtonComponent, { static: false })
+    coreRadioButton: CoreRadioButtonComponent;


make it private if you are not using it outside

fkolar · 2020-03-31T12:48:38Z

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

+     * Value for the radio-group. Should equal the value of the selected radio button if there is
+     * a corresponding radio button with a matching value.
+     */
+    protected _value: any = null;


you can make this private, I dont think you expect somebody will inhert radio

fkolar · 2020-03-31T12:53:54Z

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

+                <ng-container
+                    *ngTemplateOutlet="
+                        radio?.renderer;
+                        context: { name: name, value: radio.value, size: size, state: state, disabled: disabled }


You can disable who RadioGroup or you can disable individual RB within th e group. I think here you want to pass radio.disabed from application.

Or even better, when you set disabled to true on Group Level and its clear you want to disable all, but if it will have default value false on the group level you want read from RB definition.

disabled: disabled ||radio.disabled Does this make sense ?

as for the other attributes I guess size should be one 1, you dont dont even need to test it, it needs to have size set on the GROUP level

fkolar · 2020-03-31T12:55:42Z

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

+export class GroupRadioButtonComponent extends CollectionBaseInput implements AfterViewInit {
+    /** uniqly generated, if value not provided for id */
+    @Input()
+    id: string = `radio-group-${nextUniqueId++}`;


The same as my comment for radiobutton

fkolar · 2020-03-31T12:56:36Z

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

+     * Type of Radio buttons.
+     */
+    @Input()
+    state: stateType = 'default';


Dont we have state on BaseInput Level? or formControl level?

There is something called

export type Status = 'error' | 'warning' | void;

Where void is your default. We selected states which make sense . so you could remove state as well

if we do like this then it will diverse from fiori specification. and QA may raise more questions.

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

fkolar · 2020-03-31T13:02:34Z

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

+     * values of radio buttons. type will be of SelectItem or String
+     */
+    @Input()
+    list: Array<SelectItem | string>;


What I mean by this list list: Array<SelectItem | string>; in relation to CollectionBaseInput so to move this @Input into this parent, so we can use the same for CheckboxGroup.
Since its CollectionBaseInput, it will have this ist: Array<SelectItem | string>;.

So you could remove it from here..

fkolar · 2020-03-31T13:04:39Z

libs/platform/src/lib/components/form/data-model.ts

@@ -0,0 +1,33 @@
+/**


I though that this one is under:

https://github.com/SAP/fundamental-ngx/blob/master/libs/platform/src/lib/domain/data-model.ts

You have moved this?

fkolar · 2020-04-06T07:22:45Z

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

+    templateUrl: './radio-group.component.html',
+    changeDetection: ChangeDetectionStrategy.OnPush,
+})
+export class GroupRadioButtonComponent extends CollectionBaseInput implements AfterViewInit {


Rename to RadioGroupComponent

fkolar · 2020-04-06T07:24:38Z

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

+    exports: [GroupRadioButtonComponent],
+    declarations: [GroupRadioButtonComponent]
+})
+export class FdpRadioButtonGroupModule {}


Rename to PlatformRadioGroupModule

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

fkolar · 2020-04-07T16:06:43Z

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

+    imports: [CommonModule, FdFormModule, RadioModule, FormsModule, ReactiveFormsModule],
+    exports: [RadioButtonComponent],
+    declarations: [RadioButtonComponent]
+})


We said that we dont need this module, radio will be part of RadioGroup. Unless it make sense to use this on its own. What do you think?

fkolar · 2020-04-07T16:11:23Z

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

+    @Input()
+    get state(): Status | stateType {
+        return this._state;
+    }


Who or what will be setting a state in case of RB?

changing the style for Error, which is _status in baseInput, It needs to be passed to Radio button. so i am doing [state]="status" in "radio-group.component.html".

have removed state input and getting it from Status of BaseInput.

fkolar · 2020-04-14T19:48:44Z

@SAP/fundamental-denoland-team check you check?

naming changes name changes in test cases radio module removed removed unnecesary detectchanges

DeepakSap14 · 2020-04-16T06:12:30Z

No change needed for 0.16.0 release. so merging it.

droshev · 2020-04-17T20:58:43Z

@DeepakSap14 you should have checked with us before merging

DeepakSap14 · 2020-04-18T07:03:47Z

@droshev "@SAP/fundamental-denoland-team" , was asked for review. i waited for 1 day for any comments. then i merged.
Is any review pending, i will create separate PR, if it is needed.

DeepakSap14 requested review from fkolar and KevinOkamoto March 5, 2020 14:52

fkolar requested changes Mar 5, 2020

View reviewed changes

KevinOkamoto suggested changes Mar 5, 2020

View reviewed changes

fkolar requested review from fkolar and KevinOkamoto March 6, 2020 20:40

fkolar requested changes Mar 6, 2020

View reviewed changes

DeepakSap14 changed the title ~~feat: (platform) Radio button group component Implementation~~ feat: [WIP] (platform) Radio button group component Implementation Mar 10, 2020

DeepakSap14 changed the title ~~feat: [WIP] (platform) Radio button group component Implementation~~ [WIP]feat: (platform) Radio button group component Implementation Mar 10, 2020

DeepakSap14 changed the title ~~[WIP]feat: (platform) Radio button group component Implementation~~ [WIP] feat: (platform) Radio button group component Implementation Mar 10, 2020

DeepakSap14 force-pushed the RadioButtonGroup branch from 7f3a2bc to b80b87d Compare March 10, 2020 03:45

DeepakSap14 commented Mar 10, 2020

View reviewed changes

DeepakSap14 force-pushed the RadioButtonGroup branch from 7e7a664 to d12cf97 Compare March 10, 2020 08:00

InnaAtanasova requested a review from a team March 18, 2020 16:53

InnaAtanasova assigned DeepakSap14 Mar 18, 2020

InnaAtanasova added the platform platform label Mar 18, 2020

InnaAtanasova added this to In progress in Development via automation Mar 18, 2020

InnaAtanasova added this to the Sprint 34 - Głogów milestone Mar 18, 2020

droshev modified the milestones: Sprint 34 - Głogów, Sprint 33 - London Mar 18, 2020

KevinOkamoto approved these changes Mar 24, 2020

View reviewed changes

Development automation moved this from In progress to Review in progress Mar 29, 2020

fkolar requested changes Mar 29, 2020

View reviewed changes

DeepakSap14 requested a review from sKudum March 30, 2020 09:46

sKudum reviewed Mar 30, 2020

View reviewed changes

DeepakSap14 changed the title ~~[WIP] feat: (platform) Radio button group component Implementation~~ feat: (platform) Radio button group component Implementation Mar 30, 2020

DeepakSap14 force-pushed the RadioButtonGroup branch from e32d04c to d2ace33 Compare March 31, 2020 04:48

fkolar requested changes Mar 31, 2020

View reviewed changes

droshev modified the milestones: Sprint 33 - London, Sprint 34 - Głogów Mar 31, 2020

DeepakSap14 force-pushed the RadioButtonGroup branch 2 times, most recently from e262962 to fcc1020 Compare April 6, 2020 05:05

fkolar requested changes Apr 6, 2020

View reviewed changes

DeepakSap14 force-pushed the RadioButtonGroup branch from 5a17474 to cb54644 Compare April 7, 2020 05:21

fkolar requested changes Apr 7, 2020

View reviewed changes

DeepakSap14 force-pushed the RadioButtonGroup branch 2 times, most recently from 8ba1486 to 585f8d1 Compare April 11, 2020 10:04

Vanessa-Cusmich modified the milestones: Sprint 34 - Głogów, Sprint 35 - Nysa Apr 14, 2020

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

feat: (platform) Radio button group component Implementation #2121

Closed

fkolar approved these changes Apr 14, 2020

View reviewed changes

Development automation moved this from Review in progress to Reviewer approved Apr 14, 2020

DeepakSap14 added 4 commits April 15, 2020 12:25

search field merged from master

07559d3

build error removed

e81bb68

naming changes name changes in test cases radio module removed removed unnecesary detectchanges

status error detected

7fe8aa4

master merged for compact radio button, test fixed

8b85427

DeepakSap14 force-pushed the RadioButtonGroup branch from 585f8d1 to 8b85427 Compare April 15, 2020 08:51

DeepakSap14 merged commit cd2c26c into master Apr 16, 2020

Development automation moved this from Reviewer approved to Done Apr 16, 2020

DeepakSap14 deleted the RadioButtonGroup branch April 16, 2020 06:12

		@@ -0,0 +1,22 @@
		<ng-template #renderer>
		<ng-container *ngTemplateOutlet="radio"></ng-container>

		@@ -0,0 +1,24 @@
		import { async, ComponentFixture, TestBed } from '@angular/core/testing';

feat: (platform) Radio button group component Implementation #2122

feat: (platform) Radio button group component Implementation #2122

Conversation

DeepakSap14 commented Mar 5, 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 Mar 5, 2020

netlify bot commented Mar 5, 2020 • edited

fkolar left a comment

Choose a reason for hiding this comment

KevinOkamoto 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

KevinOkamoto commented Mar 5, 2020

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

fkolar commented Mar 24, 2020

DeepakSap14 commented Mar 24, 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

sKudum 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

fkolar Mar 31, 2020 • edited

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

fkolar commented Apr 14, 2020

DeepakSap14 commented Apr 16, 2020

droshev commented Apr 17, 2020

DeepakSap14 commented Apr 18, 2020

DeepakSap14 commented Mar 5, 2020 •

edited

netlify bot commented Mar 5, 2020 •

edited

fkolar Mar 31, 2020 •

edited