Add SelectField #114
Add SelectField #114
Conversation
dferber90
added
🎁 Type: New Component
👨🎨 Status: UI/UX Review
dferber90
force-pushed
the
df-add-select-field
branch
from
9cb0912 to
b0d00be
Compare
| 'errors', | ||
| 'isVisible', | ||
| 'renderError', | ||
| // data attributes are forwarded |
There was a problem hiding this comment.
...omit(this.props, Object.keys(SelectField.propTypes)?
There was a problem hiding this comment.
in this case doing something like const { hintIcon, badge, ..., ...rest } = this.props would have been more readable, but against our standards I suppose.
There was a problem hiding this comment.
That's pretty clever! We need to omit additional props as well like hasError (as we do not want to support it).
I think accepting any props is a bad approach as the API is not clearly defined and prop-spreading is generally hard to deal with.
To me the better approach (compared to what we're currently doing) is to be strict about the props supported in SelectInput and to remove the prop-forwarding by spread to react-select there. Then we can also be explicit about the forwarded props here.
I'd do this in a follow-up PR then though (I added it to my list).
There was a problem hiding this comment.
in this case doing something like
const { hintIcon, badge, ..., ...rest } = this.propswould have been more readable, but against our standards I suppose.
I like the explicitness of omit over "abusing" destructuring for omitting purposes, especially as we might need to declare some variables which we then won't use (like hasError).
There was a problem hiding this comment.
I now created #119 which explicitly declares all props accepted by SelectInput. I will stop the spread in SelectField (here) and forward the explicitly as well.
Then we don't need this ugly omission anymore.
dferber90
force-pushed
the
df-add-select-field
branch
3 times, most recently
from
56b50ca to
6d4c168
Compare
dferber90
changed the base branch from
master
to
df-explicitly-delcare-react-select-props
dferber90
force-pushed
the
df-add-select-field
branch
from
6d4c168 to
3b518bf
Compare
| static Placeholder = SelectInput.Placeholder; | ||
| static SelectContainer = SelectInput.SelectContainer; | ||
| static SingleValue = SelectInput.SingleValue; | ||
| static ValueContainer = SelectInput.ValueContainer; |
| touched: (props, ...rest) => | ||
| props.isMulti | ||
| ? PropTypes.arrayOf(PropTypes.bool, ...rest)(props, ...rest) | ||
| : PropTypes.bool(props, ...rest), |
| // SelectInput | ||
| 'aria-label': PropTypes.string, | ||
| 'aria-labelledby': PropTypes.string, | ||
| isAutofocussed: PropTypes.bool, |
There was a problem hiding this comment.
huh... guess I was the confused one
| id: do { | ||
| if (props.id) props.id; | ||
| else if (state.id) state.id; | ||
| else sequentialId(); |
dferber90
force-pushed
the
df-explicitly-delcare-react-select-props
branch
from
cd1eda4 to
ac0ddea
Compare
dferber90
changed the base branch from
df-explicitly-delcare-react-select-props
to
master
dferber90
force-pushed
the
df-add-select-field
branch
from
3b518bf to
44df5ee
Compare
Adds
SelectField.Needs #119 merged first.