Skip to content

Added pjax:beforeReplace cancel #428

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

Closed
wants to merge 8 commits into from
Closed

Added pjax:beforeReplace cancel #428

wants to merge 8 commits into from

Conversation

jspizziri
Copy link

Added the ability to cancel pjax on the pjax:beforeReplace event.

@mislav
Copy link
Collaborator

mislav commented Sep 3, 2014

We've discussed this in #384 (comment)

However, we're not sure what the use case of this funcionality would be?

@jspizziri
Copy link
Author

@mislav, I described my particular usecase in the following issue: #427

@mislav
Copy link
Collaborator

mislav commented Oct 14, 2014

Here's the problematic part I see with offering this functionality. You also lose these features from pjax update cycle:

  1. Ensure any elements with autofocus attribute are actually focused
  2. Ensure any <script> tags in the inserted HTML get executed
  3. Scroll to top (unless disabled)
  4. Ensure that we scroll to an element whose ID matches the fragment in the new URL

Now, I know that you either don't need or could implement these features yourself on a case-by-case basis, but the problem of offering the "cancel HTML replace" feature is that you might think you're canceling just replacing the HTML, but in fact you're canceling much more than that that is difficult to get back.

So, if we offered this feature we would also have to offer a mechanism for inserting your own HTML content and still preserving the rest of the pjax functionality.

@jspizziri
Copy link
Author

@mislav, sorry for leaving this so long.

What about allowing the user to define a callback function on pjax:beforeReplace?

like:

 var replacementHandler = fire('pjax:beforeReplace', [container.contents, options], {
    state: pjax.state,
    previousState: previousState
})

if(typeof replacementHandler === "function")
    replacementHandler(context, container.contents, options)
else
    context.html(container.contents)

This won't work exactly as written but I think you can agree/disagree on the general concept.

I feel as though this would provide a nice compromise on both fronts, by allowing the user to define their own replacement method but still maintaining the desired functionality.

@jspizziri
Copy link
Author

Some other methods in the same vein but that wouldn't require altering the fire() method are:

fire('pjax:beforeReplace', [container.contents, options, callback], {
    state: pjax.state,
    previousState: previousState
})

if(typeof callback === "function")
    callback(context, container.contents, options)
else
    context.html(container.contents)

Or simply allow a callback function to be added to the options object and check for it. This method would allow it to be added via the options object upon configuration, or upon runtime anytime up to and including pjax:beforeReplace.

fire('pjax:beforeReplace', [container.contents, options], {
    state: pjax.state,
    previousState: previousState
})

if(typeof options.replacementHandler === "function")
    options.replacementHandler(context, container.contents, options)
else
    context.html(container.contents)

@mislav mislav added the feature label Nov 21, 2014
@mislav mislav mentioned this pull request Nov 30, 2014
@jspizziri
Copy link
Author

@mislav Any word on this pull request?

@mislav
Copy link
Collaborator

mislav commented Dec 19, 2014

The problem is not "how do we let the user define their own callback". That's easy: the same user code that cancels the beforeReplace event would be responsible for running any kind of custom replacement/cleanup that may be specific to your app.

The problem is, how do we allow people to override/cancel HTML replacing but still allow them access to pjax features like the ones I listed in the above checklist. We can't possibly expect them to re-implement all 4 steps manually all the time.

@jspizziri
Copy link
Author

Perhaps you didn't see my most recent proposal, or perhaps I fail to truly understand how the pjax code is working overall. But I feel like the callback in the November 10th comment meets these needs, as it doesn't cancel pjax at all. It simply allows the user to define a replacement handler. I believe that this would provide sufficient functionality for all the use cases we have discussed.

@mislav
Copy link
Collaborator

mislav commented Dec 21, 2014

Ah yes, now I understand what you were aiming at with your example code. It's a legit solution but still not along the lines of something that I want to support. Such API just feels like an afterthought and if we start offering other customizations in the same way, it would become difficult to navigate your way around pjax usage.

If you wanted special handling for login actions, you don't need to go through all this trouble. The server can detect whether a pjax request is unauthenticated and respond with X-PJAX-URL: /login and blank body. The browser refresh to /login would then be enforced.

@jspizziri
Copy link
Author

I appreciate your hesitancy to allow something like this in pjax; however, the solution I proposed is intended to solve a much larger set of problems than just the login scenario, to which I feel there is a growing need. If you want pjax to be a viable solution for robust applications. This solution would solve issues as those mentioned here: #431 and others.

At some point, I think it is necessary to allow access to the context, content, and the replacement method for modification. And it seems to me that the best time to do this is at this point in pjax execution. That is what I'm attempting to accomplish with the proposed solution.

Thoughts?

@mislav
Copy link
Collaborator

mislav commented Dec 31, 2014

It's a technically valid solution but not the kind of API I would like to support in the long-term. I have different ideas how to allow this, but I must let them crystallize for a while first. Until this sort of feature is possible, however, I'm not convinced that developers are blocked from using pjax in "robust applications". It's more likely that the use-cases described here and in #431 are overcomplicating pjax usage. I'll keep this open in the meantime as a potential idea to gather more opinions on and as a TODO item.

@jvivs
Copy link

jvivs commented Jan 27, 2015

I'll add a 👍 for the opportunity to cancel the pjax:beforeReplace event.

I'm relatively new to pjax, but I've worked with the history API a number of times in the past.

This time around, my particular use case is pjax-enabled filtering of search results.

When a filter ends up with 0 results, we'd like to display a dialog and let the user undo the last filter selection. Before implementing ajax, we needed to implement custom state handling for these message dialogs. With history support, it's as easy as window.history.back() to restore the last valid filter.

I've added a variation of the initially suggested fix in my fork here: jvivs@7773003. Seems a bit nicer to opt out of the subsequent DOM updates when canceling pjax:beforeReplace, but still fire the pjax:complete event. It's somewhat along the lines of expected behavior of a plugin that wraps $.ajax.

After looking a bit closer, it ultimately might make more sense if a cancelable pjax:beforeReplace was fired here instead: https://github.com/jvivs/jquery-pjax/blob/master/jquery.pjax.js#L283. If we run into problems or come up with improvements, I'll swing back.

Thanks for such a solid library! It's a real breath of fresh air compared to dealing with the cross-browser quirks that come with the native history API.

@mislav
Copy link
Collaborator

mislav commented Jan 27, 2015

When a filter ends up with 0 results, we'd like to display a dialog and let the user undo the last filter selection.

Why can't that special dialog and undo button be returned by the server in case of 0 results? Then you don't have to special-case this in your frontend.

@jvivs
Copy link

jvivs commented Jan 27, 2015

@mislav Thanks for the quick reply!

It's a solution, sure. Unfortunately not as friendly for the user.

Updating the pjax container in entirety removes the previous filtered content, and thus the user's searching context. If pjax:beforeRender were cancelable, a developer would be able to opt-out of an update--in this case retain the previous context but display a dialog overlaying the content.

In order to get around those UX hangups and still take your suggestion, it would require return the previous filtered results beneath the dialog with the new url--breaking the RESTfulness of the newly-updated url.

Maybe I'm missing something, but what's the argument against being able to opt-out of a DOM manipulation? I've cruised a bunch of the pull requests surrounding this issue and it's not immediately clear how it's a bad or over-complex pattern.

I've also cruised through the pjax event flow and can't seem to find another good/better entry-point where a developer could opt out of the update if it is unsatisfactory.

@mislav
Copy link
Collaborator

mislav commented Jan 29, 2015

My reasons are in my comments above.

@Arwany Arwany mentioned this pull request Apr 14, 2015
@jspizziri
Copy link
Author

was any equivalent functionality every implemented?

@mislav
Copy link
Collaborator

mislav commented Jun 29, 2015

@jspizziri No, sorry. Working on the next big version of pjax, though.

@jvivs
Copy link

jvivs commented Jun 29, 2015

You can use my fork if you'd like.

On Mon, Jun 29, 2015 at 11:42 AM, Jacob Spizziri [email protected]
wrote:

was any equivalent functionality every implemented?


Reply to this email directly or view it on GitHub
#428 (comment).

@jspizziri
Copy link
Author

@jvivs,

Thank you, I actually have a fork of my own though, I'd just prefer to use the maintained project.

@IvoPereira
Copy link

Too bad this PR was not merged.

Gonna use your fork @jspizziri. Thanks for providing an alternative!

@jtheck
Copy link

jtheck commented Aug 16, 2016

Any word on intercepting a "back" request?

I've got a couple strictly client-side "pages" that I like to show, which replace my pjax-container (I just make it invisible to make room) - I would like to catch the "back" request to re-display the pjax-container before actually going back.

If I could preventDefault or return false from "pjax:beforeReplace" (or on pjax:start or pjax:popstate for that matter) that would be super useful.

Thanks! Love the plugin.

@RichardTMiles
Copy link

So is this a feature? could we get it documented on the readme? ;)

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants