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.

Sign up for GitHub

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

Jump to bottom

Adding the useOnlyNativeEvents (was: preferJQueryEvents) option as a way to avoid using jquery eventing in knockout. #1774

Merged
merged 4 commits into from Apr 30, 2015
Merged

Adding the useOnlyNativeEvents (was: preferJQueryEvents) option as a way to avoid using jquery eventing in knockout. #1774

merged 4 commits into from Apr 30, 2015

Conversation

dzearing
Copy link
Contributor

Usage:

<head>
    <script src="knockout.js"></script>
    <script type="text/javascript">
        ko.options.preferJQueryEvents = false;
    </script>
   <script src="jquery.js"></script>
</head>

// Now all events fired from knockout should not have jquery in the callstack. It is assumed the option is set before applyBindings is called.

@dzearing dzearing mentioned this pull request Apr 16, 2015
Closed
@SteveSanderson
Copy link
Contributor

Thanks for this!

Is it possible to add a test for this change? When we run the tests, we do so both with and without jQuery, so it would make sense to have a test that we don't get jQuery event behaviour in the presence of this flag.

@mbest
Copy link
Member

mbest commented Apr 16, 2015

Should this option also affect ko.utils.triggerEvent?

@dzearing
Copy link
Contributor Author

I'll look into the tests.

Good call @mbest. I will take a look.

@dzearing
Copy link
Contributor Author

Added a test to validate the option locks out jquery eventing.

Updated trigger.

I ran this with runner.html and runner.html?jquery=1, and validated it works in both cases. I'm not sure of a clever way to distinguish trigger behavior coming from jquery vs not.

@dzearing
Copy link
Contributor Author

ping!

@stalniy
Copy link

stalniy commented Apr 27, 2015

It's better to use direct terminology. I'd like to see useJqueryEvents = true/false instead of noJqueryEvents = true/false.

At least it's hard to understand for my brain the meaning of noJqueryEvents = false

@brianmhunt
Copy link
Member

I agree with @stalniy – The double-negative is too much for my brain. :)

The one caveat is that when useJqueryEvents is true it is not immediately obvious that the system will work even if jQuery is not present. In other words, it's almost suggesting that jQuery is either required or will be loaded or are already included in the Knockout library.

@dzearing
Copy link
Contributor Author

Renamed to "useJQueryEventing". Hmm. Maybe it should be "useJQueryEventingWhenAvailable"... or "allowJQueryEventing" or "allowJQueryEventingOverride", or maybe even a more general "allowJQueryOverrides" even though it only affects eventing... ?

@mbest
Copy link
Member

mbest commented Apr 27, 2015

When I was reading your comment, I at first thought it said "everything" instead of "eventing".

@dzearing
Copy link
Contributor Author

Happy to rename, just give me something everyone is satisified with, so we can finish this off. I can change to useJqueryEvents if it is more straightforward. In my opinion options are always going to be a little mysterious to use, even if named something completely innocent, so naming it useJQueryEvents vs useJqueryEventsWhenAvailable is still going to require a little digging to understand what that implies. It requires a little digging just to even know that jQuery is being referenced from knockout in the first place, I would have not guessed that.

@brianmhunt
Copy link
Member

@dzearing How does preferJQueryEvents ring with you?

@dzearing
Copy link
Contributor Author

Done, I like it.

@dzearing
Copy link
Contributor Author

Any other feedback?

@brianmhunt brianmhunt added this to the 3.4.0 milestone Apr 28, 2015
@dzearing dzearing changed the title Adding the noJQueryEvents option as a way to avoid using jquery eventing in knockout. Adding the preferJQueryEvents option as a way to avoid using jquery eventing in knockout. Apr 28, 2015
SteveSanderson added a commit that referenced this pull request Apr 30, 2015
@SteveSanderson
Merge pull request #1774 from dzearing/1764-jquery-eventing-disable-o…
8535832 
…ption

Adding the preferJQueryEvents option as a way to avoid using jquery eventing in knockout.
@SteveSanderson SteveSanderson merged commit 8535832 into knockout:master Apr 30, 2015
@SteveSanderson
Copy link
Contributor

Looks great! And thanks for making the option name change - sounds ideal now.

Merged. I think the other core team members will have seen this by now (since @mbest and @brianmhunt have commented here) but if not and any of you have concerns please let us know.

@rniemeyer
Copy link
Member

I thought that this one seemed fine. I would generally prefer new options to match the default/current behavior when undefined/falsey, but did agree that the double-negative was confusing.

mbest added a commit that referenced this pull request May 7, 2015
@mbest
Move DOM-related test to appropriate test file so that node tests pas…
e4c1e08 
…s. Test was added in #1774.
@mbest
Copy link
Member

mbest commented May 8, 2015

If we'd rather have an option that defaults to false, how about useOnlyNativeEvents?

@rniemeyer
Copy link
Member

If we'd rather have an option that defaults to false, how about useOnlyNativeEvents?

+1

@rniemeyer
Copy link
Member

Note: option was updated to useOnlyNativeEvents here: 09b9e93

@brianmhunt brianmhunt changed the title Adding the preferJQueryEvents option as a way to avoid using jquery eventing in knockout. Adding the useOnlyNativeEvents (was: preferJQueryEvents) option as a way to avoid using jquery eventing in knockout. Aug 30, 2015
@brianmhunt
Copy link
Member

@rniemeyer I updated the title too, just in case someone wanders onto this :)

@rniemeyer rniemeyer mentioned this pull request Aug 30, 2015
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dzearing @SteveSanderson @mbest @stalniy @brianmhunt @rniemeyer