ADDON-34289: Added Text, Radio and CheckBox components #116
Conversation
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
| } | ||
|
|
||
| handleClick = (e) => { | ||
| this.props.handleClick(this.props.id, e.target.value); |
There was a problem hiding this comment.
What would be the type of e.target.value here? , if it's 0/1 then it's fine. but if it is true/false then you need to convert before calling the handleClick as you're expecting 0/1 in this.props.value . Also, rename the function to this.props.handleChange. because all the components will receive the same function.
Also, if possible then check what's expected value on REST Handler side for Radiobox. true/false or 0/1 and call this.props.handleChange with that value ? . So later We won't need to parse value again before REST call.
There was a problem hiding this comment.
Updated funcation name to this.props.handleChange and it returns 0/1 value with field's id.
for CheckBox REST Handler uses 0/1 value. So, no need to parse the value.
for RadioBar return value would be item value provided in the GlobalConfig which will be used in REST Handler.
| value: PropTypes.string, | ||
| handleClick: PropTypes.func.isRequired, | ||
| field: PropTypes.string, | ||
| label: PropTypes.string, |
There was a problem hiding this comment.
I think, you won't need to handle label as discussed.
| onChange={this.handleChange} | ||
| value={this.props.value} | ||
| key={this.props.field} | ||
| style={{ width: 200 }} |
There was a problem hiding this comment.
just wondering, Do we explicitly need to set width here? because what if our label has long name ? If you've check with diff. scenarios then it's fine.
There was a problem hiding this comment.
Updated and used inline argument which is working as expected with different scenarios.
| inline | ||
| placeholder={this.props.controlOptions.placeholder} | ||
| className={this.props.field} | ||
| disabled={this.props.controlOptions.display === false ? true : false} |
There was a problem hiding this comment.
I think, this.props.controlOptions.display is used in form for to hide the component. That would be handle by ControlWarpper.
For, disabled you will receive this.props.disabled .
| disabled={this.props.disabled} | ||
| value={this.props.value} | ||
| onChange={this.handleChange} | ||
| type={this.props.encrypted === true ? 'password' : 'text'} |
There was a problem hiding this comment.
@dkhatri-crest No need to check this.props.encrypted === true
We can use this.props.encrypted ? 'password' : 'text'
| key={this.props.field} | ||
| value={this.props.value} | ||
| onClick={this.handleChange} | ||
| selected={this.props.value === 1 ? true : false} |
There was a problem hiding this comment.
@dkhatri-crest What should be the value of this.props.value?
If it's only 0 or 1 then we can simplify our statement like: this.props.value ? true : false
|
Expected release notes (by @dkhatri-crest) features: fixes: others (will not be included in Semantic-Release notes):
|
|
🎉 This PR is included in version 4.4.0-develop.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.0.0-develop.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
ADDON-34289
Added Text, Radio and CheckBox components