yiisoft / jquery-pjax

pushState + ajax = pjax
http://pjax.herokuapp.com
MIT License
143 stars 40 forks source link

Added `skipOuterContainers` option #29

Closed SilverFire closed 8 years ago

SilverFire commented 8 years ago

This PR adds skipOuter option.

The use case:

<div class="container" id="pjax-container">
    Go to <a href="/page/2">next page</a>.
    <div class="status-container" id="pjax-status-container">
        <span>Last check found no errors on page 1</span>
        <a href="/update-status/page/1">Check again</a>
    </div>
</div>
<script>
    $(document).pjax('a', '#pjax-container');
    $(document).pjax('a', '#pjax-status-container', {
        push: false,
        replace: false,
        timeout: 0
    })
</script>

I want to use a link inside div#pjax-container to navigate between pages with pushing URLs to the history. Every page displays some info and has a button Check again to update that info without page refresh or pushing to the browser history. As you see, I placed the button inside new container and defined a special config for it.

Without out this PR, the config of #pjax-status-container is useless, because the network request of #pjax-status-container will be immediately aborted by #pjax-container. Then #pjax-container will create his own request and process it according to it's rules.

When skipOuter = true only the closest container will handle the request.

The options defaults to false, so it doesn't break BC.

SilverFire commented 8 years ago

@nkovacs I know you are familiar with pjax. Could you take a look, please?

samdark commented 8 years ago

Name sounds a bit too vague to me. The logic of the change is OK.

SilverFire commented 8 years ago

There were two other names, that I tried:

When anyone has better name - please, text it!

samdark commented 8 years ago

skipOuterContainers sounds much better to me even if it's longer.

SilverFire commented 8 years ago

Okay, let it be skipOuterContaiters

nkovacs commented 8 years ago

There are some fixes to nested containers and pjax's cache upstream. It's also possible Yii's widget is initializing the pjax code incorrectly.

SilverFire commented 8 years ago

There are some fixes to nested containers

Yeah, it's time to merge with origin. @samdark ?

It's also possible Yii's widget is initializing the pjax code incorrectly.

Why?

nkovacs commented 8 years ago

There are various ways to configure pjax, e.g. you can add data-pjax="container-selector" on links to specify a container instead of doing it in the $.pjax call. Maybe some combination of these makes nested containers work, and another breaks them. I'd try reproducing this with the upstream version, without Yii, first.

nkovacs commented 8 years ago

Looking at the pjax code: it calls preventDefault: https://github.com/defunkt/jquery-pjax/blob/master/jquery.pjax.js#L98 and it checks for isDefaultPrevented: https://github.com/defunkt/jquery-pjax/blob/master/jquery.pjax.js#L83

So the outer container should receive the event with isDefaultPrevented, and abort.

SilverFire commented 8 years ago

@nkovacs You think we should call event.preventDefault() instead on return here?

SilverFire commented 8 years ago

Will merge this if nobody complains

SilverFire commented 8 years ago

@nkovacs I checked it. It's not the same.

SilverFire commented 8 years ago

https://github.com/yiisoft/jquery-pjax/commit/ef12a6bddadbb64445b3001821cfa0314a246fe7