-
Notifications
You must be signed in to change notification settings - Fork 5
TNC component #124
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
base: main
Are you sure you want to change the base?
TNC component #124
Conversation
| name={name} | ||
| readOnly={readOnly} | ||
| disabled={!enabled} | ||
| disabled={context.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.
What if checkbox is rendered without TnC?
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 this logic and implemented it via model, fetching the model of checkbox and enabling it after scroll.
| appliedCssClassNames, | ||
| readOnly | ||
| } = props; | ||
| const options = enumNames && enumNames.length ? enumNames : enums || []; |
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.
Why does link component have checkbox group, its not clear ?
|
|
||
| return ( | ||
| <div | ||
| className={`cmp-adaptiveform-checkboxgroup cmp-adaptiveform-checkboxgroup--${ |
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.
Why are we not re-using the existing checkbox group component ?
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.
@rismehta, the bem is different when compared to checkbox group component as it contains the anchor tag so that one can visit the link in order to enable the checkbox. The same approach has been used in core component as well.
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 don't see the BEM different, can you point out the differences ? For core components, re-usability was not trivial, thats why we did not re-use the components. But in react, you should be able to re-use, if there are differences expose them as props.
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.
Implemented, re-used the checkbox group component!
rismehta
left a comment
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.
check comments
| key={enums![index]} | ||
| > | ||
| <label className="cmp-adaptiveform-checkboxgroup__option-label"> | ||
| <a |
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.
@rismehta we are using anchor tag here.
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: