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

Adds tag creator #151

Merged
merged 1 commit into from
May 18, 2015
Merged

Adds tag creator #151

merged 1 commit into from
May 18, 2015

Conversation

flovilmart
Copy link
Contributor

Adds allowCreate=true prop so the field will allow option creation.
By default this option is disable to maintain backward compatibility

@dcousens
Copy link
Collaborator

Hi @flovilmart, could you please update this PR without the dist files? (and rebase if necessary)
I think this is a cool addition in any case :)

@flovilmart
Copy link
Contributor Author

Ok. I'll do my best :)

On Mon, May 11, 2015 at 8:47 PM, Daniel Cousens notifications@github.com
wrote:

Hi @flovilmart, could you please update this PR without the dist files?

I think this is a cool addition in any case :)

Reply to this email directly or view it on GitHub:
#151 (comment)

@dcousens dcousens added this to the 0.4.9 milestone May 12, 2015
@dcousens dcousens self-assigned this May 12, 2015
@flovilmart flovilmart force-pushed the creator branch 3 times, most recently from 2fc80ac to 50adf7a Compare May 12, 2015 01:17
@flovilmart
Copy link
Contributor Author

Rebased and removed the commits from the dist and lib

@bsr203
Copy link

bsr203 commented May 13, 2015

+1

@beeant beeant mentioned this pull request May 13, 2015
@beeant
Copy link

beeant commented May 13, 2015

+1

@flovilmart
Copy link
Contributor Author

I've updated, and rebased

On Wed, May 13, 2015 at 1:37 PM, Bryant Teja notifications@github.com
wrote:

+1

Reply to this email directly or view it on GitHub:
#151 (comment)

@dcousens
Copy link
Collaborator

Thanks @flovilmart this is great.

dcousens added a commit that referenced this pull request May 18, 2015
@dcousens dcousens merged commit 3345d4a into JedWatson:master May 18, 2015
@dcousens
Copy link
Collaborator

@flovilmart should a callback be fired to notify of a new tag? We should probably try to lift more state out of this component.

@flovilmart
Copy link
Contributor Author

Not that implémentation, just the on change is fired. That would be a nice feature (but I didn't need it at the time)

On Mon, May 18, 2015 at 2:53 AM, Daniel Cousens notifications@github.com
wrote:

@flovilmart should a callback be fired to notify of a new tag?

Reply to this email directly or view it on GitHub:
#151 (comment)

@dcousens
Copy link
Collaborator

@flovilmart thoughts about making a PR for it? :)

@flovilmart
Copy link
Contributor Author

Haha :) you could do it too :p what callback signature would you like? 

Would you want if before or after the change?

Do you want it cancellable?

On Mon, May 18, 2015 at 7:18 PM, Daniel Cousens notifications@github.com
wrote:

@flovilmart thoughts about making a PR for it? :)

Reply to this email directly or view it on GitHub:
#151 (comment)

@dcousens
Copy link
Collaborator

@flovilmart haha, before the state change is always better, that way the user can throw new props at it if necessary.

Do you want it cancellable?

That is a good question, if this module was structured properly, we'd handle this like an onChange event for a typical field and manage the state in the parent, but, at least right now, the state is being managed internally... :(

@flovilmart
Copy link
Contributor Author

As for thé signature, I could go with the same as onChange.

Call it onAdd?

On Mon, May 18, 2015 at 7:46 PM, Daniel Cousens notifications@github.com
wrote:

@flovilmart haha, before the state change is always better, that way the user can throw new props at it if necessary.

Do you want it cancellable?

That is a good question, if it was structured properly, we'd handle this like an onChange event for a typical field and manage the state in the parent, but in this case, the state is being managed internally...

Reply to this email directly or view it on GitHub:
#151 (comment)

@dcousens
Copy link
Collaborator

@flovilmart sounds good to me

@flovilmart flovilmart mentioned this pull request May 19, 2015
@flovilmart
Copy link
Contributor Author

Just created another PR, maybe we should rename allowCreate to enableAdd, or canAdd or allowAdd, or addEnabled...

@dcousens
Copy link
Collaborator

canAdd works for me.

@dcousens dcousens mentioned this pull request Jun 3, 2015
@nickbouton
Copy link

+1

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.

5 participants