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

please bring back _registerInternalEvents to enable plugins to override them #4984

Closed
asyraf9 opened this Issue Sep 13, 2012 · 10 comments

Comments

Projects
None yet
4 participants
@asyraf9

asyraf9 commented Sep 13, 2012

hi guys, this is more of a request than an issue.

before JQM 1.1, all form submit, click, and hashChanges were defined in a single object called $.mobile._registerInternalEvents. this was called upon init to bind all the necessary navigation events to allow JQM's page navigations to happen.

now in the latest JQM version, all these handlers are bound in a callback that is tied to $.mobile.navReadyDeferred. this is awesome too, but i don't see a way for me to unbind or override these handlers, or even to remove the callback from the deferred and put mine in - except maybe, to do stopPropagation in my handler - which can be extremely buggy.

I love the _registerInternalEvents implementation - it allowed me to completely rewrite all the navigation event handlers in my splitview plugin - which was important, since I'm working on multiple pageContainers at one time.

so, I would like to propose that we bring that back, or namespace all the event handlers, or if you have a better idea how i can overcome this issue without changing core - even more welcome. thanks for the awesome work guys.

@asyraf9

This comment has been minimized.

Show comment
Hide comment
@asyraf9

asyraf9 Sep 13, 2012

btw, the change is fairly small, I can issue a pull request should that be necessary

asyraf9 commented Sep 13, 2012

btw, the change is fairly small, I can issue a pull request should that be necessary

@jaspermdegroot

This comment has been minimized.

Show comment
Hide comment
@jaspermdegroot

jaspermdegroot Sep 17, 2012

Member

@asyraf9 - Would be great if you can create a PR for this that we can review.

Member

jaspermdegroot commented Sep 17, 2012

@asyraf9 - Would be great if you can create a PR for this that we can review.

@asyraf9

This comment has been minimized.

Show comment
Hide comment
@asyraf9

asyraf9 Sep 17, 2012

not a problem. i'll send in a pull request by day after tomorrow.

sent from my phone
On Sep 17, 2012 10:00 PM, "Jasper de Groot" notifications@github.com
wrote:

@asyraf9 https://github.com/asyraf9 - Would be great if you can create
a PR for this that we can review.


Reply to this email directly or view it on GitHubhttps://github.com/jquery/jquery-mobile/issues/4984#issuecomment-8615165.

asyraf9 commented Sep 17, 2012

not a problem. i'll send in a pull request by day after tomorrow.

sent from my phone
On Sep 17, 2012 10:00 PM, "Jasper de Groot" notifications@github.com
wrote:

@asyraf9 https://github.com/asyraf9 - Would be great if you can create
a PR for this that we can review.


Reply to this email directly or view it on GitHubhttps://github.com/jquery/jquery-mobile/issues/4984#issuecomment-8615165.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 21, 2012

Contributor

@johnbender

I guess we can do this fairly easily like this.

Contributor

gabrielschulhof commented Sep 21, 2012

@johnbender

I guess we can do this fairly easily like this.

@asyraf9

This comment has been minimized.

Show comment
Hide comment
@asyraf9

asyraf9 Sep 21, 2012

tried that. apparently deferred.done() stores a reference to the callback
function, so changing $.mobile._registerInternalEvents after registering it
to the deferred makes no difference, because the deferred already has the
original callback registered.

I had to bind the callback registration
(deferred.done($.mobile._registerInternalEvents)) to the mobileinit event
to allow overriding.

pull request in a bit

sent from my phone
On Sep 21, 2012 7:46 PM, "Gabriel |Nix| Schulhof" <
notifications@github.com> wrote:

@johnbender https://github.com/johnbender

I guess we can do this fairly easily like thishttps://gist.github.com/3760126
.


Reply to this email directly or view it on GitHubhttps://github.com/jquery/jquery-mobile/issues/4984#issuecomment-8762116.

asyraf9 commented Sep 21, 2012

tried that. apparently deferred.done() stores a reference to the callback
function, so changing $.mobile._registerInternalEvents after registering it
to the deferred makes no difference, because the deferred already has the
original callback registered.

I had to bind the callback registration
(deferred.done($.mobile._registerInternalEvents)) to the mobileinit event
to allow overriding.

pull request in a bit

sent from my phone
On Sep 21, 2012 7:46 PM, "Gabriel |Nix| Schulhof" <
notifications@github.com> wrote:

@johnbender https://github.com/johnbender

I guess we can do this fairly easily like thishttps://gist.github.com/3760126
.


Reply to this email directly or view it on GitHubhttps://github.com/jquery/jquery-mobile/issues/4984#issuecomment-8762116.

@gabrielschulhof

This comment has been minimized.

Show comment
Hide comment
@gabrielschulhof

gabrielschulhof Sep 23, 2012

Contributor

I'd rather not change when we attach the done callback. Instead, let's do it like this. If the function stored at $.mobile._registerInternalEvents has changed by the time the done callback executes, then we shall have this nailed.

Contributor

gabrielschulhof commented Sep 23, 2012

I'd rather not change when we attach the done callback. Instead, let's do it like this. If the function stored at $.mobile._registerInternalEvents has changed by the time the done callback executes, then we shall have this nailed.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Sep 24, 2012

Contributor

@asyraf9

This puts us in a really tough spot.

On the one hand the change that @gabrielschulhof has proposed is fairly trivial and it appears as though it would help you out.

On the other hand this is a private method and we are already at the release candidate stage. 1.2 was in beta for something like a month when we could have made this change without concern. Again, it seems really trivial but making any code change during an RC is a risk.

It looks like user's of your plugin will have to wait for the next patch release to upgrade :(

Contributor

johnbender commented Sep 24, 2012

@asyraf9

This puts us in a really tough spot.

On the one hand the change that @gabrielschulhof has proposed is fairly trivial and it appears as though it would help you out.

On the other hand this is a private method and we are already at the release candidate stage. 1.2 was in beta for something like a month when we could have made this change without concern. Again, it seems really trivial but making any code change during an RC is a risk.

It looks like user's of your plugin will have to wait for the next patch release to upgrade :(

@ghost ghost assigned gabrielschulhof Sep 24, 2012

@asyraf9

This comment has been minimized.

Show comment
Hide comment
@asyraf9

asyraf9 Sep 24, 2012

@gabrielschulhof loved that solution. thought about it yesterday and began testing it but saw that you'd beaten me to it here :) thanks. i'll issue a new PR for 1.2.1

@johnbender not to worry - please proceed with releasing 1.2 without this. I'll provide a modified version in my repo. sorry i hadn't asked for this earlier - i haven't been using my own plugin for a while, but each time jqm goes to beta or RC, my inbox gets full - had to do something heh.

asyraf9 commented Sep 24, 2012

@gabrielschulhof loved that solution. thought about it yesterday and began testing it but saw that you'd beaten me to it here :) thanks. i'll issue a new PR for 1.2.1

@johnbender not to worry - please proceed with releasing 1.2 without this. I'll provide a modified version in my repo. sorry i hadn't asked for this earlier - i haven't been using my own plugin for a while, but each time jqm goes to beta or RC, my inbox gets full - had to do something heh.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Sep 24, 2012

Contributor

@asyraf9

I should have added that the only reason it puts us in a rough spot is that we care about supporting community plugins. We really can't handle every use case internally and so we rely heavily on people like you to fill in the gaps.

We'll push to get 1.2.1 out quick in the meantime, and thanks again for contributing.

Contributor

johnbender commented Sep 24, 2012

@asyraf9

I should have added that the only reason it puts us in a rough spot is that we care about supporting community plugins. We really can't handle every use case internally and so we rely heavily on people like you to fill in the gaps.

We'll push to get 1.2.1 out quick in the meantime, and thanks again for contributing.

@asyraf9

This comment has been minimized.

Show comment
Hide comment
@asyraf9

asyraf9 Sep 24, 2012

You guys have been great. It's been a pleasure really. thanks!

asyraf9 commented Sep 24, 2012

You guys have been great. It's been a pleasure really. thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment