Skip to content
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

AsyncCreatable HOC #1210

Merged
merged 7 commits into from
Sep 13, 2016
Merged

AsyncCreatable HOC #1210

merged 7 commits into from
Sep 13, 2016

Conversation

bvaughn
Copy link
Collaborator

@bvaughn bvaughn commented Sep 12, 2016

Resolves issue #1200.

Summary

Combining Async and Creatable HOCs turned out to be more tricky than I had initially expected. It is not sufficient to simple nest one within the other. To this end, I propose a new AsyncCreatable HOC that neatly ties the other two together (keeping the complexity out of user-land).

In general, async and composition is an area that we should probably put a lot of thought into before version 2 ships (so as to avoid some of the hackiness in this PR). Maybe this is good enough for v1 though?

I've updated the example site so that the Github users (Async) demo has a "creatable?" checkbox that can be used to test the Async + Creatable combination.

Syntax

import React from 'react';
import { AsyncCreatable } from 'react-select';

function render (props) {
  return (
    <AsyncCreatable {...props} />
  );
}

Demo

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 91.334% when pulling 3ae25c3 on 1200-continued into f723189 on master.

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 12, 2016

Pfft. Coveralls is soooo pessimistic. 😜

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 92.033% when pulling 79ffa35 on 1200-continued into f723189 on master.

bvaughn referenced this pull request Sep 12, 2016
Following a child-function pattern. Tests have been added.
@traumatic
Copy link

AsyncCreateable works great for me. Maybe it would be worth implementing "Home/Pos1" and "End" keys. Great work Brian!

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 13, 2016

@traumatic Thanks for giving it a spin!

Maybe it would be worth implementing "Home/Pos1" and "End" keys. Great work Brian!

This seems unrelated to this HOC, or am I misunderstanding?

@traumatic
Copy link

Sorry @bvaughn! Stupid me, the focus is on the dropbox list one I start typing. All good.

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 13, 2016

No problem at all. Just wanted to clarify. 😄

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 92.033% when pulling feb70e4 on 1200-continued into f723189 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 92.033% when pulling 3a5c5ed on 1200-continued into f723189 on master.

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 13, 2016

@JedWatson: I feel good about this PR after smoke testing it for a while. (@traumatic also smoke tested it.) I'm going to move forward with it since it cleans up the slightly awkward state PR #1200 left master in. Let's chat on Twitter if you have any concerns or remaining suggestions. 😄

@bvaughn bvaughn merged commit d09a267 into master Sep 13, 2016
@bvaughn bvaughn deleted the 1200-continued branch September 13, 2016 08:24
@stinoga
Copy link

stinoga commented Sep 13, 2016

@bvaughn FYI, I've been testing this with our react select implementation all morning, and haven't found any issues. Both the areas updated to use AsyncCreatable and our other react select components are working great.

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 13, 2016

Thanks for the update @stinoga! That's great to hear. 😁

@rudeayelo
Copy link

Seems like this one was merged but it wasn't built before the release, so it's in the /src but not in /dist, therefore not published to npm 😭

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 14, 2016

It's more recent than RC1. It should go out with RC2. 😄

@rudeayelo
Copy link

Ups, sorry, my bad 😓

@deepakbansal1010
Copy link

Hey @bvaughn Any tentative timeline for RC2 release?

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 17, 2016

No. I reached out to Jed about it on Twitter. If he's okay with us making another RC, I think it would be good to. However I don't think I have permissions to publish the release to Npm myself. Will see.

@ansonkao
Copy link

Wow looks like I'm the first person to try using AsyncCreatable before it's been published. What are the chances!

+1 for this!

@bvaughn
Copy link
Collaborator Author

bvaughn commented Sep 18, 2016

We talked. Looks like RC2 will land this weekend. I'm trying to finish a refactor of Async to fix a few issues first. If I'm not able to finish it in time we'll release without it.

@deepakbansal1010
Copy link

Hey @bvaughn
I'm using react-select component in one of my angular app with ECMA 5 syntax.
Everything was working fine until I introduced AsyncCreatable component.
In my app there are multiple react-select component used in a modal window. When I come to the modal window for the first time, the options populated are correct for all the multi select.
However when I save the details and open the same modal window again for adding one more records, the all multi select options are messed up and get populated with random values.

@bvaughn
Copy link
Collaborator Author

bvaughn commented Oct 14, 2016

Sorry to hear you're having some troubles with the component, @deepakbansal1010. I'm afraid I don't have enough info to help based on your description though. You will likely have better luck if you include a link to a Plnkr (or similar) that reproduces the problem? You may also find faster responses on Stack Overflow since a lot more people check it than here. 😁

@MilllerTime
Copy link

@bvaughn Thanks for all the effort put into this component! AsyncCreatable is working well, until I add a custom filterOptions prop. At that point async lookup still works, however the creatable behavior disappears - there is no longer an option to add an unrecognized value.

The custom filter I'm using works as expected with both the Creatable hoc and the base Select component. The issue is only with AsyncCreatable. My filter is just removing options a user previously selected, but which react-select is no longer aware of.

Interestingly, I'm even observing this behavior with a simple identity function:

filterOptions={options => options}

Is there something to be aware of regarding this combination, or possibly an internal issue with AsyncCreatable preventing the use of a custom options filter?

If necessary, I can whip up a working demo to reproduce.

@bvaughn
Copy link
Collaborator Author

bvaughn commented Feb 11, 2017

Is there something to be aware of regarding this combination, or possibly an internal issue with AsyncCreatable preventing the use of a custom options filter?

AsyncCreatable is very small so it's likely not an internal issue with it. At a glance, I would expect it to work as well as it works with Creatable. Sounds like a bug? File a GH issue if you think it is. 😄 Repros are always nice for GH issues.

@MilllerTime
Copy link

You're right, it looks like relatively simple composition. There's now a link to a plunker that demonstrates the bug in #1547

@bvaughn
Copy link
Collaborator Author

bvaughn commented Feb 12, 2017

Hi 😄 If you haven't already, please file this as a GH issue. We're talking on a PR that was merged some time ago and so it's likely to get overlooked.

@MilllerTime
Copy link

Hey, totally agree. I wanted to connect this to the GH issue since I started the discussion here. So that was a link to the issue, not just the plunker. Sorry for any confusion 🙂

@bvaughn
Copy link
Collaborator Author

bvaughn commented Feb 13, 2017 via email

@sarathantony
Copy link

@bvaughn : hello, i'm new to this. i tried, but couldn't be able to implement this. i'm struck in this more than 3 days. please help me out. i'm using it in an atomic design env. it's okey, i can handle that. my requirement is async+creatable+multi please help me out with useage of { AsyncCreatable } (can it really meet my requirements?)with all necessary props.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants