-
Notifications
You must be signed in to change notification settings - Fork 54
Adds missing props to InputBase types #228
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
Adds missing props to InputBase types #228
Conversation
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.
One thing I noticed, the TextField
didn't automatically get these like I thought it would--it extends FormControl.publicProps
instead. Should I also add these to TextField.props
explicitly or is there a different approach you all would prefer?
Yes, please spread them into the TextField props if possible. There might be an overlap though. |
@fhammerschmidt I've pushed a WIP commit that shows one way we could accomplish this, by creating more 'partial' record types and composing them more directly where they're being used |
| @as("soft") Soft | ||
| @as("off") Off | ||
|
||
type inputBaseComponentPublicProps = { |
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.
IMO, this should probably be in CommonProps
for inputs / textarea 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.
Yes, please move them.
fbfa51b
to
8996322
Compare
Apart from the conflicts and the commented-out props that got overridden (I think you can just remove them), is this ready? |
57c7ee8
to
e11b18c
Compare
/** | ||
* The maximum value the user can enter in the input. See [MDN](https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/max) for more details. | ||
*/ | ||
max?: 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.
The min
and max
props technically are not just int
, maximum/minimum values can be other values too: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/max
Additionally the step
is probably better off as a float
: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/step#syntax
Since this is a breaking change, should I leave it out? Otherwise I can make it an unboxed type that is string | float
e: due to the timezone difference I've elected to remove the breaking change so you can merge if you wish. I can follow up in another PR if that's a desired change
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.
Yeah it mainly depends on the type and thus is probably a good fit for unboxed.
Let's merge this first.
e11b18c
to
d600f6d
Compare
400a5b8
to
a2e137d
Compare
`InputBase.inputBaseComponentProps` was moved to `CommonProps.inputTextareaProps` and added: 1. `minLength` 2. `maxLength` 3. `wrap` `InputBase.publicProps` added: 1. `maxLength` 2. `minLength` 3. `wrap` `TextField.props` now includes the input / text area specific props `CommonProps.globalAttributes` now holds the HTML attributes that are valid for all elements. `CommonProps.clickableProps` was renamed to `CommonProps.eventHandlerProps` and added the following event handlers: 1. onChange 1. onBeforeInput 1. onInput 1. onReset 1. onSubmit 1. onInvalid resolves cca-io#224
aa917bd
to
8128cd0
Compare
InputBase.inputBaseComponentProps
was moved toCommonProps.inputTextareaProps
and added:minLength
maxLength
wrap
InputBase.publicProps
added:maxLength
minLength
wrap
TextField.props
now includes the input / text area specific propsCommonProps.globalAttributes
now holds the HTML attributes that are valid for all elements.CommonProps.clickableProps
was renamed toCommonProps.eventHandlerProps
and added the following event handlers:resolves #224