-
Notifications
You must be signed in to change notification settings - Fork 77
docs: Call out FormInput's props explicitly #471
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
Conversation
| type: PropTypes.string, | ||
| value: PropTypes.string | ||
| }; | ||
|
|
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.
prop description should be provided for all non default props seen here:
src/Forms/FormInput.js
Outdated
|
|
||
| FormInput.propDescriptions = { | ||
| disabled: 'Set to **true** to mark component as disabled and make it non-interactive.', | ||
| readOnly: 'Set to **true** to mark component as readonly.', |
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.
those are default props - their descriptions will be assigned automatically by https://github.com/SAP/fundamental-react/blob/master/src/_playground/documentation/Properties/defaults.js
You will need add descriptions here for name, state, and arguably type as the definition differs 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.
Ok, I want to make sure I understand you correctly.
The descriptions at https://github.com/SAP/fundamental-react/blob/master/src/_playground/documentation/Properties/defaults.js are automatically added.
What I need to provide are the descriptions for the props (name, value) that are not described in the above file as well as modify type's description to reflect the FormInput component.
greg-a-smith
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.
@truclyle There should not be any changes in the package-lock.json file since there are no changes in the package.json file. It's likely a version mismatch that's causing it. We are using Node Version Manager and running Node 10.15.0, which comes bundled with NPM 6.4.1.
greg-a-smith
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.
Looks good. 🚢 Thanks for the contribution!
Description
Call out more of FormInput's props explicitly for PropType validation using FormRadioItem as a model.
fixes #464