-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
React Select v2 #2289
React Select v2 #2289
Conversation
I just published v2 significantly improves on v1 in several areas:
Major work to do before v2 hits feature parity with v1 includes:
It also needs complete documentation and an upgrade guide with changes from v1.x, as well as a lot of tests (and user testing). In the meantime, if you're interested read the example source to see how the new API works. |
Quick update: I published Getting closer! |
Amazing work! We're starting a new project using v2, looking great so far 💯 Only thing so far is that I can't seem to get the |
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.
Added few suggestions, nothing mayor found yet.
src/Async.js
Outdated
componentDidMount() { | ||
this.mounted = true; | ||
const { defaultOptions } = this.props; | ||
if (defaultOptions === true) { |
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.
I found this defaultOptions
behavior a bit weird. I was expecting it to be falsy or an array, not loading options on mount if true
. I know you are saving a prop but I'm wondering if using a dedicated one as in v1 makes the behavior more predictable
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.
I think you're right, it does seem a bit less predictable this way. Happy to add another prop.
src/Async.js
Outdated
() => { | ||
this.loadOptions(inputValue, options => { | ||
if (!this.mounted) return; | ||
if (options) { |
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.
What abut checking also cacheOptions
? If it is false then it can skip caching the response altogether
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 that seems like a good thing to do
src/Select.js
Outdated
}; | ||
}; | ||
|
||
options.forEach((item, itemIndex) => { |
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.
Instead of forEach
I would use reduce
, so render
is no longer a side effect but it is the result of the loop
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.
@albertogasparin would you be interested in creating a PR for that?
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.
Sure. Here is my take #2359
"classnames": "^2.2.4", | ||
"prop-types": "^15.5.8", | ||
"react-input-autosize": "^2.1.2" | ||
"classnames": "^2.2.5", |
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.
Is this still required? Cannot see it being used
.TODO.md
Outdated
|
||
* [ ] Prevent values from being popped, was `option.clearableValue === false` | ||
* [ ] Scroll behaviour: should we detect parent? how do we handle the footer? | ||
* [ ] Scroll behaviour: can we overscroll up to show the group heading? |
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.
Another one is: can the user customize scrollIntoView
? For instance, if the option group header has position: sticky
then the scroll position should account for its height
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, we should also do some work around handling more complex requirements for rendering the menu (e.g into a Portal)
fontSize: 'inherit', | ||
opacity: isHidden ? 0 : 1, | ||
outline: 0, | ||
padding: 0, |
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.
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.
@jossmac can you take a look at this?
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.
@JedWatson sure thing, I'll check it out tomorrow.
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.
@albertogasparin @JedWatson resolved in 07bba99 of #2354
@Zertz great to hear! I've just implemented Promise support in Async, it'll be available in the upcoming @albertogasparin thanks for the review 👌 some good points in there. I'm currently working on a pretty big cleanup (see #2351) and will work through your feedback as soon as I land that. |
Fantastic work @JedWatson ! We're releasing |
Could you release an alpha, it's been a while. |
What about TypeScript typings? BTW This is an amazing project! |
Heads up everyone who's watching this thread -
I'll consider that after the final release of 2.0.0, I know they're helpful but I'm reluctant to introduce anything that needs to be kept in sync after the experience of maintaining both less and scss in v1.x, where only less was actually used by the examples and build. I'm not sure what's worse between no types, and out of sync types. |
Thanks for the great effort @JedWatson !! v2 looks really promising. I can't wait to use it on my next project. |
Universal/SSR is broken due to https://github.com/JedWatson/react-select/blob/v2/src/utils.js line 206 I am pulling down locally and working on a patch, will submit PR as soon as I've got SSR working |
Great news - React Select 2.0.0-beta.1 is out 🎉 At this point, I think the rewrite is basically complete and don't expect much to change before general release of 2.0. So, I need help making sure it's ready 😃 I'd like to keep it in beta for a couple of weeks, so there's time to get feedback and make sure the upgrade guide / docs are ready before we merge it into Thanks for picking up SSR @falconmick, if anyone else finds issues during beta please open an issue (or even better, PR) with the prefix [v2] in the title. |
Export indicator icons for use in customizations
I’ll not be able to get onto the issue until late Sunday or early Monday (Aussie time) |
Added `closeMenuOnScroll` prop
remove deprecation warning and backward compatibility logic
[V2] Default filterOption prop in Async component instead of passing null
…ts and innerRef/innerProps
V2 update issue template
V2 redistribute inner ref
Prefer more descriptive esm word instead of es
update contributing md
update changelog in prep for v2.0.0 release
OMFG 🎉 |
Updated: See the preview here: http://bit.ly/react-select-v2
v2 issue for discussion: #2208
View the v2 source: https://github.com/JedWatson/react-select/tree/v2
Alpha releases of react-select@2.0.0 are available on npm for preview, and I have opened this PR to help increase visibility of the current state, ci status and netlify preview.
v2 should still be considered experimental, but it's at a point where I would absolutely welcome feedback on the architecture and approach. More examples and documentation coming soon.
PRs and issues specific to the new version should be tagged
v2