Upgrade to react-select v2 #58
Conversation
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
- Coverage 100% 99.13% -0.87%
==========================================
Files 59 60 +1
Lines 542 581 +39
Branches 118 128 +10
==========================================
+ Hits 542 576 +34
- Misses 0 5 +5
Continue to review full report at Codecov.
|
furious-luke
force-pushed
the
techdebt/react-select-v2
branch
from
75f8b4d
to
35d3e11
Compare
furious-luke
requested review from
scaredcat and
tabby-or-not
and removed request for
scaredcat
| value={44} | ||
| valueKey="value" | ||
| /> | ||
| <div |
There was a problem hiding this comment.
Ideally we'd like to keep this mocked to <Select.Async>
There was a problem hiding this comment.
Yup. Can create a mock for the new Select.js component as well to cover this case, similar to:
https://github.com/uptick/react-object-list/blob/master/__mocks__/react-select.js
There was a problem hiding this comment.
As recommended by @scaredcat I've updated the react-select mock to provide a better name for Select.Async.
| } | ||
| } | ||
|
|
||
| return nextProps |
There was a problem hiding this comment.
if I try
const props = {
value: 101010
}
const transform = {
value: ['value', val => parseInt(val.toString(), '2')]
}
convertProps(props, transforms) // {val: 101010}since KNOWN_PROPS overwrites the transform of 'value' back to the original value.
this function would only work if a value transform does not save it back to the original prop key
There was a problem hiding this comment.
Yep, that's the intended behavior. Anything that needs a transform would be removed from KNOWN_PROPS; it's just a way of avoiding errors being thrown out for props that don't need a transform.
| value={44} | ||
| valueKey="value" | ||
| /> | ||
| <div |
There was a problem hiding this comment.
Yup. Can create a mock for the new Select.js component as well to cover this case, similar to:
https://github.com/uptick/react-object-list/blob/master/__mocks__/react-select.js
| @@ -1,6 +1,6 @@ | |||
| import React from 'react' | |||
| import PropTypes from 'prop-types' | |||
| import Select from 'react-select' | |||
| import Select from '../../utils/Select' | |||
There was a problem hiding this comment.
this is used below to determine propTypes. Happy to simply change the proptype check to a catch all propTypes.object and removing this line.
There was a problem hiding this comment.
@scaredcat This should be working now; I've added react-select prop types into the wrapper.
|
I was thinking I could at least change the background of the select input to white to make it look close to the previous version. I'll also run it past the product team. |
|
@scaredcat @tabby-or-not Okay, I've patched up a few of the issues raised, how do we feel about it now? |
Workforce is upgrading to
react-selectv2, and so we need to bumpreact-object-listtoo. I've used the same approach as for Workforce; create a thin wrapper that manages converting the API from v1 to v2 for us, so we don't have to change each usage ofreact-select.I've noticed that a couple of strange warnings appear in the console as a result of prop types:
It looks like we're passing undefined to a prop type definition somewhere, but that seems pretty strange to me. Probably need someone more familiar with how this package works to take a look.
There were a lot of snapshot tests that needed updating, I went through them and couldn't spot anything wrong.