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 onAdd handler #197

Closed
wants to merge 4 commits into from
Closed

Adds onAdd handler #197

wants to merge 4 commits into from

Conversation

flovilmart
Copy link
Contributor

As per discussion on #151

@dcousens
Copy link
Collaborator

Looks good to me.

Thanks @flovilmart

@dcousens
Copy link
Collaborator

Actually, can you change allowCreate to canAdd?

@flovilmart
Copy link
Contributor Author

Yep. I'll do that. BTW The onAdd fires with the same parameters as onChange.

On Tue, May 19, 2015 at 9:47 PM, Daniel Cousens notifications@github.com
wrote:

Actually, can you change allowCreate to canAdd?

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

@dcousens
Copy link
Collaborator

@flovilmart excellent :)

Select: fix inline documentation formatting/spelling
@dcousens
Copy link
Collaborator

bump @flovilmart

@flovilmart
Copy link
Contributor Author

Bump?

@dcousens
Copy link
Collaborator

Actually, can you change allowCreate to canAdd?

See flovilmart#2

@dcousens
Copy link
Collaborator

dcousens commented Jun 5, 2015

@flovilmart anything holding this back?

@dcousens dcousens mentioned this pull request Jun 5, 2015
@brianreavis
Copy link
Collaborator

As mentioned in #235, an onAdd event should should be paired with an onRemove, right? Or is this more of an onCreate event? Sorry, I'm a little confused on the terminology here. With the "multi" mode, an onAdd event that really represents when new items are created is going to create confusion.

@flovilmart
Copy link
Contributor Author

I agree that onCreate would be more appropriate

On Fri, Jun 5, 2015 at 9:26 PM, Brian Reavis notifications@github.com
wrote:

As mentioned in #235, an onAdd event should should be paired with an onRemove, right? Or is this more of an onCreate event? Sorry, I'm a little confused on the terminology here. With the "multi" mode, an onAdd event that really represents when new items are created is going to create confusion.

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

@brianreavis
Copy link
Collaborator

@dcousens Your thoughts? @flovilmart Sorry for suggesting we go back and forth on this!

@flovilmart
Copy link
Contributor Author

No problem. That's a quick fix and it's already on production with my fork :) better do it now!

On Fri, Jun 5, 2015 at 10:35 PM, Brian Reavis notifications@github.com
wrote:

@dcousens Your thoughts? @flovilmart Sorry for suggesting we go back and forth on this!

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

@dcousens
Copy link
Collaborator

dcousens commented Jun 6, 2015

@brianreavis excellent point, onCreate and onRemove sound fine.

I'll close my PR, @flovilmart are you happy to fix this soon? If not, I'll merge and manually fix. 👍

Thanks for this.

@flovilmart
Copy link
Contributor Author

Il do it tomorrow.also, the flag should be back to canCreate :)

On Fri, Jun 5, 2015 at 11:16 PM, Daniel Cousens notifications@github.com
wrote:

@brianreavis excellent point, onCreate and onRemove sound fine.
I'll close my PR, @flovilmart are you happy to fix this soon? If not, I'll merge and manually fix. 👍

Thanks for this.

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

@dcousens
Copy link
Collaborator

Bump @flovilmart,

@seanmheff
Copy link

Bump

@agirton
Copy link
Collaborator

agirton commented May 20, 2017

Closing as this is no longer applicable.

@agirton agirton closed this May 20, 2017
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