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

Fixes issue #9 - added unique event name "showandtell" #10

Merged
merged 1 commit into from Jun 15, 2017

Conversation

ViRuSTriNiTy
Copy link

@ViRuSTriNiTy ViRuSTriNiTy commented Jun 14, 2017

... and updated tests (children.js still shows issues)

see issue #9

- updated tests (children.js still shows issues)
@hypesystem
Copy link
Owner

This looks pretty good. Are the tests still passing?

I think I might prefer the events to be namespaced, eg. showandtell.remove instead of remove (and also instead of your proposed showandtell with reason: remove). What do you think about that?

Thanks for taking the time to make a PR :-)

@ViRuSTriNiTy
Copy link
Author

All tests are passing but i can't get children.js to work. Would you mind taking a look what`s the issue there?

As for the namespaced event name, i initialy opted towards this solution but i found some issues with it like:

https://stackoverflow.com/questions/4718841/namespaced-custom-events-trigger

Maybe i should just try it out, perhaps this issue isn't there anymore.

@hypesystem
Copy link
Owner

Fair, let's go with the model for event naming you have implemented for now.

What output is children.js giving you?

@hypesystem
Copy link
Owner

Oh! I think maybe children.js never worked, so you're ok leaving it broken :-) (I know this is really bad practice, but it is as-yet unfixed...)

@hypesystem
Copy link
Owner

I will just merge your code :-)

@hypesystem hypesystem merged commit 781d585 into hypesystem:master Jun 15, 2017
@ViRuSTriNiTy
Copy link
Author

Side note: i've tested namespace events with jQuery 2.1.4 and jQuery 3.0, seems to work properly, so the stackoverflow issue does not exist anymore.

@ViRuSTriNiTy
Copy link
Author

Another side note: the children.js test was never working as you added these tests in an issue discussion, see here: #7 (comment)

@hypesystem
Copy link
Owner

Cool. I think maybe we should go for the namespaced events before we make a release, if there aren't any issues with them? What do you say?

@ViRuSTriNiTy
Copy link
Author

Namespace incoming in PR #12, see also issue #11

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.

None yet

2 participants