TextInput: refresh adornment API #770
Conversation
… bugs and add deprecation warnings for TextInput
There was a problem hiding this comment.
🙌 🙌 🙌 Thanks @stvtortora!
I've left a few comments and questions, but this is looking pretty good at the moment! The main one was about the backwards compatibility, since it looks like the padding values may be different (and we do use some hand-measured values for this API in aragon/aragon and aragon-apps).
Do changes need to be made where this component is utilized throughout your other repositories?
If you'd like to—by all means, we'd appreciate it! Otherwise we may end up upgrading this along with a few other changes published in the last few versions of aragonUI :).
As a heads up, I accidentally added the changes from #769
All good, I've merged this branch with master!
| /> | ||
| <TextInput /> | ||
| <TextInput adornment={{ start: <IconBlank /> }} /> | ||
| <TextInput adornment={{ end: <IconBlank /> }} /> | ||
| <TextInput | ||
| css="width: 80px" | ||
| adornment="ETH" |
There was a problem hiding this comment.
For these older examples, perhaps we can either remove them or update them to use the new API?
| @@ -154,8 +208,6 @@ WrapperTextInput.propTypes = { | |||
| WrapperTextInput.defaultProps = { | |||
| ...TextInput.defaultProps, | |||
| adornment: null, | |||
There was a problem hiding this comment.
I'm not sure, but should we add null to the list of allowed types for adornment? Not sure how PropTypes.oneOf() handles this, but I could see it erroring.
There was a problem hiding this comment.
Sadly, React doesn't seem to have null as an available propType. :(
There was a problem hiding this comment.
Prop types interpret null and undefined as the same thing, so it shouldn’t be a problem as long as isRequired is not used. 👍
To combine a value with a type, the trick is to use PropType.oneOf() with only the value, making it a standard PropType type:
PropTypes.oneOfType([
PropTypes.oneOf([null]),
PropTypes.number,
])This is what we are doing for the aragonUI’s _null prop type.
src/components/Input/TextInput.js
Outdated
| const usingDeprecatedAPI = | ||
| React.isValidElement(adornment) || | ||
| typeof adornment === 'string' || | ||
| (typeof adornment === 'object' && adornment.constructor === Array) |
There was a problem hiding this comment.
I would also assume a user wants the old API if they use adornmentPosition and adornmentSettings, and widen the PropTypes.node check a bit:
E.g. something like:
(React.isValidElement(adornment) || typeof adornment !== 'object' || Array.isArray(adornment)) ||
adornmentPosition ||
adornmentSettingsThe main part that changes for the PropTypes.node check is changing typeof adornment === 'string' to typeof adornment !== object. PropTypes.node covers numbers and potentially other renderable types.
src/components/Input/TextInput.js
Outdated
| position: relative; | ||
| width: ${props.wide ? '100%' : 'max-content'}; | ||
| ${start && `padding-left: ${startWidth}`} | ||
| ${end && `padding-right: ${endWidth}`} |
There was a problem hiding this comment.
It looks like the backwards compatibility above might not reproduce this exactly, since it just uses the adornmentSettings.padding as the padding.
| @@ -154,8 +208,6 @@ WrapperTextInput.propTypes = { | |||
| WrapperTextInput.defaultProps = { | |||
| ...TextInput.defaultProps, | |||
| adornment: null, | |||
| adornmentPosition: 'start', | |||
| adornmentSettings: {}, | |||
| } | |||
|
|
|||
| // <input type=number> (only for compat) | |||
There was a problem hiding this comment.
Could we also update this component's documentation with the new changes? We can remove (or mark) the deprecated props :).
sohkai
changed the title
Co-authored-by: Brett Sun <qisheng.brett.sun@gmail.com>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for contributing to Aragon! 🦅 |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for contributing to Aragon! 🦅 |
Fixes #656
Do changes need to be made where this component is utilized throughout your other repositories?
As a heads up, I accidentally added the changes from https://github.com/aragon/aragon-ui/pull/769 to this PR because I initially forgot to create a new branch.