Attatch pjax events to injected links? #34

New Issue

Closed

opened 2015-01-07 21:28:37 -05:00 by pklada · 25 comments

pklada commented 2015-01-07 21:28:37 -05:00 (Migrated from github.com)

Copy Link

Should the pjax events attach to links that are injected through a previous pjax call by default or do you have to do something special to attach new pjax events to those newly inserted links? Currently, even though the links I am inserting follow the selector defined in elements, they are causing hard refreshes (not pjax events).

Should the pjax events attach to links that are injected through a previous pjax call by default or do you have to do something special to attach new pjax events to those newly inserted links? Currently, even though the links I am inserting follow the selector defined in `elements`, they are causing hard refreshes (not pjax events).

pklada commented 2015-01-07 21:29:48 -05:00 (Migrated from github.com)

Copy Link

This is my init code:

new Pjax({
  elements: "a.js-pjax"
, selectors: ["title", "[name=analytics_page_name]", ".content", ".pjax-custom-js", ".pjax-mixpanel-track"]
});

new Pjax({
  elements: ".sct a"
, selectors: ["title", "[name=analytics_page_name]", ".hero", "main", ".pjax-custom-js", ".pjax-mixpanel-track"]
});

(note that I have two different pjax implementations, not sure if this would cause the issue.)

This is my init code: ``` new Pjax({ elements: "a.js-pjax" , selectors: ["title", "[name=analytics_page_name]", ".content", ".pjax-custom-js", ".pjax-mixpanel-track"] }); new Pjax({ elements: ".sct a" , selectors: ["title", "[name=analytics_page_name]", ".hero", "main", ".pjax-custom-js", ".pjax-mixpanel-track"] }); ``` (note that I have two different pjax implementations, not sure if this would cause the issue.)

MoOx commented 2015-01-08 02:01:11 -05:00 (Migrated from github.com)

Copy Link

You can just execute this code for every page, links that already get a event listener will be ignored (so everything will work "as expected".

I should update the readme with this example:

// edit: code removed because of the stupidity of the anwser

You can just execute this code for every page, links that already get a event listener will be ignored (so everything will work "as expected". I should update the readme with this example: ``` js // edit: code removed because of the stupidity of the anwser ```

pklada commented 2015-01-08 10:18:36 -05:00 (Migrated from github.com)

Copy Link

Ah, okay, good to know. I was unsure if calling pjax on the same element groups would effectively double the attached events. Thanks!

Ah, okay, good to know. I was unsure if calling pjax on the same element groups would effectively double the attached events. Thanks!

MoOx commented 2015-01-08 10:19:50 -05:00 (Migrated from github.com)

Copy Link

Hum reopening to let people know that trick while is not documented

Hum reopening to let people know that trick while is not documented

pklada commented 2015-01-08 10:20:26 -05:00 (Migrated from github.com)

Copy Link

Note that I believe the docs say explicitly not to do this :p

Note that I believe the docs say explicitly _not_ to do this :p

pklada commented 2015-01-08 10:21:21 -05:00 (Migrated from github.com)

Copy Link

Also IMO this is probably something that should be done by the library by default. I can hack at it later when time permits :)

Also IMO this is probably something that should be done by the library by default. I can hack at it later when time permits :)

MoOx commented 2015-01-08 10:27:10 -05:00 (Migrated from github.com)

Copy Link

Haha my bad. I am stupid. The library should do that by default a17a6b90be/src/pjax.js (L467-L469)
So you should not do what I put in the example above indeed. I'll edit the comment to remove this snippet.

Haha my bad. I am stupid. The library should do that by default https://github.com/MoOx/pjax/blob/a17a6b90bebefd8f5209e6a6f7d8c5d59296232a/src/pjax.js#L467-L469 So you should not do what I put in the example above indeed. I'll edit the comment to remove this snippet.

MoOx commented 2015-01-08 10:28:11 -05:00 (Migrated from github.com)

Copy Link

Take a look here how I'm doing on my website 40e2146b84/src/assets/_scripts/vanilla.js

Take a look here how I'm doing on my website https://github.com/MoOx/moox.github.io/blob/40e2146b8483a7d05fd578c5a99cd136b24a65a9/src/assets/_scripts/vanilla.js

MoOx commented 2015-01-08 10:28:51 -05:00 (Migrated from github.com)

Copy Link

So this should work by default is selector are ok. It should already reapply to all the selector (& skip the elements already affected)

So this should work by default is selector are ok. It should already reapply to all the selector (& skip the elements already affected)

MoOx commented 2015-01-08 10:29:37 -05:00 (Migrated from github.com)

Copy Link

Are you using 0.1.4 ?

Are you using 0.1.4 ?

pklada commented 2015-01-08 11:34:38 -05:00 (Migrated from github.com)

Copy Link

I am. I did some testing, it appears it works fine if both of my pjax calls use the same selectors, but as soon as i define different selectors per init, it seems to cause issues. I will continue to investigate.

You can see what I mean if you go to http://guidebook.com; clicking around the sub-nav works fine, clicking a main-nav link works fine, but if you click on a main-nav link and then return to the index and then use the sub-nav, it will cause a full reload. Not sure why.

I am. I did some testing, it appears it works fine if both of my pjax calls use the same `selectors`, but as soon as i define different `selectors` per init, it seems to cause issues. I will continue to investigate. You can see what I mean if you go to http://guidebook.com; clicking around the sub-nav works fine, clicking a main-nav link works fine, but if you click on a main-nav link and then return to the index and _then_ use the sub-nav, it will cause a full reload. Not sure why.

pklada commented 2015-01-08 14:56:51 -05:00 (Migrated from github.com)

Copy Link

Also, just tested that re-init'ing pjax on pjax:complete does in fact cause the links in question to work again, but the pjax events are bound/fired multiple times, so that doesn't work as a solution.

Also, just tested that re-init'ing pjax on pjax:complete does in fact cause the links in question to work again, but the pjax events are bound/fired multiple times, so that doesn't work as a solution.

MoOx commented 2015-01-10 01:55:23 -05:00 (Migrated from github.com)

Copy Link

I think I get it. For now only one instance is reparsing DOM for its own selectors, not for other pjax instances. So maybe you should use a pjax event to for reparsing for other part. I did do anything special to handle this case. Maybe we should create a pjax.refresh() method to force reload.
It seems like an edge case to use 2 instances that can update same part of the page :/

I think I get it. For now only one instance is reparsing DOM for its own selectors, not for other pjax instances. So maybe you should use a pjax event to for reparsing for other part. I did do anything special to handle this case. Maybe we should create a pjax.refresh() method to force reload. It seems like an edge case to use 2 instances that can update same part of the page :/

pklada commented 2015-01-10 18:01:35 -05:00 (Migrated from github.com)

Copy Link

@MoOx cool, thanks for looking into it. I can take a stab at writing that method if I have time today.

@MoOx cool, thanks for looking into it. I can take a stab at writing that method if I have time today.

pklada commented 2015-01-11 20:45:51 -05:00 (Migrated from github.com)

Copy Link

@MoOx I'm going to take a crack at this. Whats the best way to get this repo up and running locally so I can hack at it? Do you just browserify the index.js file? Let me know what your setup looks like. Thanks!

@MoOx I'm going to take a crack at this. Whats the best way to get this repo up and running locally so I can hack at it? Do you just browserify the index.js file? Let me know what your setup looks like. Thanks!

MoOx commented 2015-01-12 00:38:49 -05:00 (Migrated from github.com)

Copy Link

According to the package.json scripts section, you should just use npm test command (after npm install).
The current repo state is kind of broken (some tests aren't good because last time I work on this repo I didn't finish what I was working).

I started a refactoring (one big file to many files with tests) that I didn't finish.

What should be cool is to work with low level tests. You will probably need to extract some part of the code into new files (because I didn't finish the entire refactoring).
Take a look to this PR https://github.com/MoOx/pjax/pull/33 that did that kind of stuff.

Tell me if that's enough for you to start.

According to the package.json scripts section, you should just use `npm test` command (after `npm install`). The current repo state is kind of broken (some tests aren't good because last time I work on this repo I didn't finish what I was working). I started a refactoring (one big file to many files with tests) that I didn't finish. What should be cool is to work with low level tests. You will probably need to extract some part of the code into new files (because I didn't finish the entire refactoring). Take a look to this PR https://github.com/MoOx/pjax/pull/33 that did that kind of stuff. Tell me if that's enough for you to start.

pklada commented 2015-01-12 02:43:26 -05:00 (Migrated from github.com)

Copy Link

@MoOx thanks. After playing around with it today I'm not sure the repo is in a working enough state for me to just go in and fix this issue. It seems like its kind of in a broken state, unfortunately :(

I might want to modify the existing version which is hosted on npm and update that to fix this issue. Is that possible?

@MoOx thanks. After playing around with it today I'm not sure the repo is in a working enough state for me to just go in and fix this issue. It seems like its kind of in a broken state, unfortunately :( I might want to modify the existing version which is hosted on npm and update that to fix this issue. Is that possible?

MoOx commented 2015-01-12 02:50:28 -05:00 (Migrated from github.com)

Copy Link

You can do that too. Just find a way to share the fix so I can bring it back in there :)

You can do that too. Just find a way to share the fix so I can bring it back in there :)

pklada commented 2015-01-12 03:51:01 -05:00 (Migrated from github.com)

Copy Link

@MoOx pretty simple: https://github.com/pklada/pjax/compare/MoOx:master...master

Just added a simple exposed refresh method which would refresh the document based on the elements in the options. Also added some logic so that calling this wouldnt double-bind pjax events. In the code on my side I just call this on pjax:complete.

var mainPjax = new Pjax({
  elements: "a.js-pjax"
, selectors: ["title", "[name=analytics_page_name]", ".content", ".pjax-custom-js", ".pjax-mixpanel-track"]
});

var sctPjax = new Pjax({
  elements: ".sct a"
, selectors: ["title", "[name=analytics_page_name]", ".hero", "main", ".pjax-custom-js", ".pjax-mixpanel-track"]
});

$(document).on('pjax:complete', function(){
  mainPjax.refresh();
  sctPjax.refresh();
});

Maybe not the most elegant solution, but it works.

@MoOx pretty simple: https://github.com/pklada/pjax/compare/MoOx:master...master Just added a simple exposed refresh method which would refresh the document based on the elements in the options. Also added some logic so that calling this wouldnt double-bind pjax events. In the code on my side I just call this on pjax:complete. ``` var mainPjax = new Pjax({ elements: "a.js-pjax" , selectors: ["title", "[name=analytics_page_name]", ".content", ".pjax-custom-js", ".pjax-mixpanel-track"] }); var sctPjax = new Pjax({ elements: ".sct a" , selectors: ["title", "[name=analytics_page_name]", ".hero", "main", ".pjax-custom-js", ".pjax-mixpanel-track"] }); $(document).on('pjax:complete', function(){ mainPjax.refresh(); sctPjax.refresh(); }); ``` Maybe not the most elegant solution, but it works.

MoOx commented 2015-01-13 01:00:36 -05:00 (Migrated from github.com)

Copy Link

Good to know. Thanks for sharing I'll handle that next time I'll continue the refactoring.

Good to know. Thanks for sharing I'll handle that next time I'll continue the refactoring.

pklada commented 2015-01-13 13:26:35 -05:00 (Migrated from github.com)

Copy Link

@MoOx what are the plans w/ the refactor? Honestly I might move the current master to a new branch since its in a sort of unfinished state, maybe a refactor branch, and then roll back the current branch to .1.4. I can help in my free time as well as this library was really helpful to me, Id just like to keep the npm package, etc, up to date.

@MoOx what are the plans w/ the refactor? Honestly I might move the current master to a new branch since its in a sort of unfinished state, maybe a refactor branch, and then roll back the current branch to .1.4. I can help in my free time as well as this library was really helpful to me, Id just like to keep the npm package, etc, up to date.

MoOx commented 2015-01-13 13:36:51 -05:00 (Migrated from github.com)

Copy Link

The current master should work in a Commonjs env. If not, it should not be a big deal to fix since I mainly just move some code (split the big file into smaller tested pieces).

I were working on a branch but was not going forward enough so I decide to break things to move fast.
I understand your arguments but think we should iterate on the current master.

Sorry to not be in a hurry, I'm really busy with bigger OS projects for now (like cssnext or stylelint) but totally want to keep this repo up since I'm using it for my website (I plan to refactor it soon, so I'll end up to use an up to date version of this lib as well).

Note: I recently give write access to @igierard yesterday since he is interested to & have made a nice PR: #33.

Maybe we should just add more minimal tests to ensure the current master works (note that the broken test for now has been made on purpose, as a reminder to finish the refactoring) so we can release new version (reminder we are still under v1.0.0, so we can totally break things ^^).

The current master _should_ work in a Commonjs env. If not, it should not be a big deal to fix since I mainly just move some code (split the big file into smaller tested pieces). I were working on a branch but was not going forward enough so I decide to break things to move fast. I understand your arguments but think we should iterate on the current master. Sorry to not be in a hurry, I'm really busy with bigger OS projects for now (like cssnext or stylelint) but totally want to keep this repo up since I'm using it for my website (I plan to refactor it soon, so I'll end up to use an up to date version of this lib as well). Note: I recently give write access to @igierard yesterday since he is interested to & have made a nice PR: #33. Maybe we should just add more minimal tests to ensure the current master works (note that the broken test for now has been made on purpose, as a reminder to finish the refactoring) so we can release new version (reminder we are still under v1.0.0, so we can totally break things ^^).

pklada commented 2015-01-13 13:39:30 -05:00 (Migrated from github.com)

Copy Link

^ okay, that's fair. I updated my project w/ my forked version for now. (guidebook.com if you want to check it out working) Thanks.

^ okay, that's fair. I updated my project w/ my forked version for now. (guidebook.com if you want to check it out working) Thanks.

MoOx commented 2015-01-13 13:40:43 -05:00 (Migrated from github.com)

Copy Link

If you are interested to contribute to the project, just make a nice PR as a start & we will see what happen :)

If you are interested to contribute to the project, just make a nice PR as a start & we will see what happen :)

pklada commented 2015-02-04 01:56:31 -05:00 (Migrated from github.com)

Copy Link

Closing as my fixes got added to master.

Closing as my fixes got added to master.

Sign in to join this conversation.

No Branch/Tag Specified

Branches Tags

master

dependabot/npm_and_yarn/simple-git-and-lint-staged--removed

dependabot/npm_and_yarn/qs-6.5.3

dependabot/npm_and_yarn/decode-uri-component-0.2.2

dependabot/npm_and_yarn/jsdom-16.5.0

dependabot/npm_and_yarn/shell-quote-1.7.3

dependabot/npm_and_yarn/cached-path-relative-1.1.0

dependabot/npm_and_yarn/path-parse-1.0.7

dependabot/npm_and_yarn/hosted-git-info-2.8.9

dependabot/npm_and_yarn/elliptic-6.5.4

dependabot/npm_and_yarn/ini-1.3.7

dependabot/npm_and_yarn/serve-10.1.2

dependabot/npm_and_yarn/mixin-deep-1.3.2

dependabot/npm_and_yarn/eslint-utils-1.4.2

dependabot/npm_and_yarn/js-yaml-3.13.1

dependabot/npm_and_yarn/extend-3.0.2

feature/yarn

0.2.8

0.2.7

0.2.6

0.2.5

0.2.4

dev

0.2.3

0.2.2

0.2.1

0.2.0

0.1.4

0.1.3

v0.1.2

v0.1.1

v0.1.0

Labels

Clear labels

bug

dependencies

Pull requests that update a dependency file

duplicate

enhancement

invalid

question

wontfix

No Label enhancement

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

iLoveElysia

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: iLoveElysia/pjax#34