-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: rewrite Checkbox as a separate module | suit to Fiori3 #1777
feat: rewrite Checkbox as a separate module | suit to Fiori3 #1777
Conversation
Deploy preview for fundamental-ngx ready! Built with commit 26be28b |
return this.checkboxState === 'indeterminate'; | ||
} | ||
|
||
writeValue(value: any): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's mark methods, that are implemented from ControlValueAccessor
as /** @hidden */
, they shouldn't be in documentation IMO. Let's shortly describe all of other methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
code: formGroupInputTs, | ||
fileName: 'checkbox-form-group-example', | ||
component: 'CheckboxFormGroupExampleComponent' | ||
language: 'TypeScript', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typescript should have lower case 't' for stackblitz to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
ngOnInit() { } | ||
checkboxReactiveForms: ExampleFile[] = [ | ||
{ | ||
language: 'TypeScript', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
checkboxStates: ExampleFile[] = [ | ||
{ | ||
language: 'TypeScript', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
]; | ||
|
||
checkboxCustomValues: ExampleFile[] = [ | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add the html as well for all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
expect(checkbox.checkboxValue).toBe(false); | ||
}); | ||
|
||
// it('should be unchecked on double click', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are all of these comments for. IF they are necessary remove the ones with nothing inside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test cases have been updated
|
||
/** Values returned by control. */ | ||
@Input() | ||
values: FdCheckboxValues = {trueValue: true, falseValue: false, thirdStateValue: null}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know by doing this we are giving them flexibility but shouldn't a checkbox be strictly a boolean ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkbox control returns boolean values by default.
I think giving user possibility to define values returned by checkbox is a nice and useful feature.
* */ | ||
public nextValue(): void { | ||
|
||
if (this.disabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
d28c0be
to
1199c9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very well for me. Great Job, the compare-object
function will be useful in feature..
874683b
to
ec07905
Compare
I complete agree with Jedrzej . Great job! |
The build is failing. Could you take a look at it? |
ec07905
to
0a33c97
Compare
…kbox using compareObjects
…ocumentation examples using old versions of checkbox
0a33c97
to
7c1d534
Compare
* Working cheeckbox implementation * Create documentation examples * Add checkbox interface annotations * feat(core): update unit tests * feat(core) Update checkbox api description * core(feat) Working unit tests for checkbox * feat(core) Introduce compareObjects function | Compare values in checkbox using compareObjects * core(feat) Review changes * feat(core) Remove checkbox support in FormControlDirective | Update documentation examples using old versions of checkbox * feat(core) fix circular dependencies * feat(core) Add custom label example | Add files indexing * feat(core) Change encapsulation and change detection strategy * feat(core) Make MultiInput tests async
Please provide a link to the associated issue.
closes #1769
Please provide a brief summary of this pull request.
This pull requests includes:
Checkbox state is driven by value passed to component by FormControl.
Please check whether the PR fulfills the following requirements
Documentation checklist: