Allow non-pjax page updates. by ndelage · Pull Request #684 · twbs/ratchet

ndelage · 2014-10-01T14:53:16Z

Extract transition functionality from push.js into a new file, transitions.js.
Expose a global function, similar to PUSH named TRANSITION used to transition
from one page/view to another without dictating how the new markup is sourced.

This allows for Ratchet style page transitions when rendering client side.

I'll add comments to the diff shortly, highlighting some of the changes I've made.

ndelage · 2014-10-01T15:03:22Z

I didn't see any reason to keep the list of bar element selectors in an object (see the key/value pairs at the top of push.js). So it's a simple list of selectors now.

cvrebert · 2014-10-04T00:19:34Z

@ndelage This is failing in Travis: https://travis-ci.org/twbs/ratchet/builds/36779341

ndelage · 2014-10-04T00:32:27Z

Yeah, I noticed. The linter doesn't think TRANSITION has been defined. But in the context of a full Ratchet codebase, it is defined on window (as part of transitions.js). I haven't put too much effort into satisfying the linter yet. I figure we can cross that bridge once we've discussed the patch.

I'm welcome to any suggestions if you've dealt with a similar issue before.

hnrch02 · 2014-10-04T00:40:38Z

Change /* global _gaq: true */ to /* global _gaq, TRANSITION */ to fix that.

ndelage · 2014-10-10T17:24:44Z

Thanks, Travis is passing now.

avinoamr · 2014-10-11T21:44:07Z

@ndelage Suggest to modify this line as follows:

document.body.innerHTML = ""; document.body.appendChild( contents );

This will ensure that all bound events and data will remain attached to the element instead of "flattening" it to just HTML string

ndelage · 2014-10-12T01:38:35Z

Thanks for the comments @avinoamr, these are spot on. I agree with moving TRANSITIONto PUSH. I wasn't too excited to expose another global. That said, it seems that Ratchet would benefit from a single namespace, but that's another discussion entirely.

I'll submit the relevant changes in the next day or so.

avinoamr · 2014-10-12T20:29:18Z

@ndelage If transition is set, you end up calling complete() twice - once here and again when the transition completes.

I've removed the premature (before the transition completes) call to complete. In my version transitionContent is only called when a transition is present.

ndelage · 2014-10-13T02:50:27Z

Now appending content instead setting innerHTML.

hnrch02 · 2014-10-22T00:08:32Z

Pinging @connor for a review.

ndelage · 2014-10-22T00:53:01Z

I'd love some feedback, but given the potential that we move to a module approach with a single top-level object/namspace Ratchet, I think it makes sense to move the new TRANSITION function into the push.js IIFE. This way we avoid adding another global.

Later if we move towards a single Ratchet object, it will be easy to expose the transition functionality via a property of Ratchet (e.g. Ratchet.transition)

Tarang · 2014-10-26T09:11:30Z

Cool PR. It would definitely help our project. I've not dived deep in, but is it able to take DOM over raw html?

ndelage · 2014-10-26T19:05:11Z

Yep, that's the idea. The TRANSITION function takes the markup & transition style as arguments. I've been using this with a Backbone app (allowing me to avoid PJAX)

Tarang · 2014-10-27T10:10:08Z

Cool! I just gave this ago but I seem to have a couple of problems. I tried to use TRANSITION(domElement, 'slide-out') or slide-in. If I remove the transition it swaps the page out. With the transition I can see the transition going on, but the new page seems to disappear and I'm remaining with the old page, albeit without the titlebar.

ndelage · 2014-11-08T04:01:49Z

Hey @Tarang, sorry for the delay. I just pushed an update, including auto-generated js & css in dist/. The update allows for both HTML markup as a string or DOM elements as the contents argument to TRANSITION. Here's example usage, passing a string:

var contents =   "<div><div class='bar-tab'></div><div class='bar-nav'></div><div class='contents'<p>Foo Bar Baz</p></div></div>";
TRANSITION(contents, 'slide-left');

The single wrapper <div> needs to be included. It could be another element (like <body>) if necessary. But the contents argument needs to represent a single element.

Give it another shot and let me know if you run into any issues.

yenbuilds · 2014-11-08T09:11:39Z

Is anybody else noticing a memory leak on transitions?

ndelage · 2014-11-08T15:46:42Z

Could you test master for the same leak? Can you provide details on how you've been testing for memory leaks?

ndelage · 2014-11-08T16:44:06Z

Just added a step to clone the contents argument to TRANSITION (I discovered it was being modified when applying a transition).

ndelage · 2014-11-08T17:45:39Z

Disclaimer: I'm no expert in tracking down memory leaks or profiling JS. I ran some tests using Chrome's timeline, tracking memory usage over time and forcing garbage collection a few times throughout the test. After each GC, it looks like everything has been cleaned up. I don't see a memory increase:

yenbuilds · 2014-11-09T11:03:48Z

Thanks Nate for the reply! I looked deeper into it and it seems like it may be a weird jQuery XHR issue, and not related to the TRANSITION call. I also looked at the DOM through the remote Safari debugger and it looks like everything is being cleaned up correctly. So sorry about that.

cocoademon · 2015-01-05T03:59:42Z

Is this likely to be merged?
I'd love to switch from using my current workaround, which is having a bunch of empty .html and injecting template contents in an on('push') handler.

XhmikosR · 2015-02-04T23:40:07Z

@ndelage: can you rebase and clean up this branch?

luisrudge · 2015-02-07T05:50:23Z

+11111111 to this :)

XhmikosR · 2015-02-09T04:13:20Z

@ndelage: please fetch and rebase.

ndelage · 2015-02-09T15:57:41Z

Will do. I'll get to this in the next few days.

Extract transition functionality from push.js into a new file, transitions.js. Expose a global function, similar to PUSH named TRANSITION used to transition from one page/view to another without dictating how the new markup is sourced. This allows for Ratchet style page transitions when rendering client side.

Ensure that all bound events and data will remain attached to the element instead of "flattening" it to just HTML string.

ndelage · 2015-03-12T15:24:49Z

Just pushed a rebased branch, switching over to the new window.RATCHET global. Going to do a bit of testing locally, but feel free to have another look at the pr.

XhmikosR · 2015-03-12T16:02:58Z

We don't need the dist files in the PR to reduce conflicts
Many of the patches can be squashed

ndelage · 2015-03-12T16:16:51Z

Roger. I'll make those changes.

qkdreyer · 2015-06-30T07:25:58Z

Any updates ?

ndelage reviewed Oct 1, 2014
View reviewed changes

This was referenced Oct 1, 2014

Extract DOM push.js transitions #683

Open

Single page support? #127

Open

hnrch02 added js feature labels Oct 1, 2014

ndelage force-pushed the extract-transitions branch from 5b441a9 to 51ccdf4 Compare October 10, 2014 16:44

avinoamr reviewed Oct 11, 2014
View reviewed changes

avinoamr reviewed Oct 12, 2014
View reviewed changes

ndelage force-pushed the extract-transitions branch from f31fe28 to 1637b4f Compare October 13, 2014 02:51

ndelage force-pushed the extract-transitions branch from a4c77f5 to f95101c Compare November 9, 2014 22:37

ndelage added 10 commits March 12, 2015 09:42

Remove unused variables

669c762

Let JSHint know TRANSITION is a global

053dfca

Whitespace fixes

0ed5ef3

Remove premature callback execution

b36d858

Append content to DOM instead of setting innerHTML

ed585a0

Ensure that all bound events and data will remain attached to the element instead of "flattening" it to just HTML string.

Include transitions.js

d5a93ce

Support HTML string as argument to TRANSITION fn

3ac0b8c

Update dist & docs after grunt build

d2709f1

Clone node on transition

0961870

ndelage force-pushed the extract-transitions branch from f95101c to 0961870 Compare March 12, 2015 14:50

ndelage added 2 commits March 12, 2015 10:08

Move TRANSITION over new RATCHET global

004ee04

Satisfy the linter

9217aee

XhmikosR force-pushed the master branch from 1c7d2fa to 7d7ef88 Compare August 20, 2015 15:08

amakh mentioned this pull request Dec 9, 2015

Ember support for push.js #808

Closed

Uh oh!

Conversation

ndelage commented Oct 1, 2014

Uh oh!

ndelage Oct 1, 2014

Choose a reason for hiding this comment

Uh oh!

cvrebert commented Oct 4, 2014

Uh oh!

ndelage commented Oct 4, 2014

Uh oh!

hnrch02 commented Oct 4, 2014

Uh oh!

ndelage commented Oct 10, 2014

Uh oh!

avinoamr Oct 11, 2014

Choose a reason for hiding this comment

Uh oh!

ndelage commented Oct 12, 2014

Uh oh!

avinoamr Oct 12, 2014

Choose a reason for hiding this comment

Uh oh!

ndelage Oct 13, 2014

Choose a reason for hiding this comment

Uh oh!

ndelage commented Oct 13, 2014

Uh oh!

hnrch02 commented Oct 22, 2014

Uh oh!

ndelage commented Oct 22, 2014

Uh oh!

Tarang commented Oct 26, 2014

Uh oh!

ndelage commented Oct 26, 2014

Uh oh!

Tarang commented Oct 27, 2014

Uh oh!

ndelage commented Nov 8, 2014

Uh oh!

yenbuilds commented Nov 8, 2014

Uh oh!

ndelage commented Nov 8, 2014

Uh oh!

ndelage commented Nov 8, 2014

Uh oh!

ndelage commented Nov 8, 2014

Uh oh!

yenbuilds commented Nov 9, 2014

Uh oh!

cocoademon commented Jan 5, 2015

Uh oh!

XhmikosR commented Feb 4, 2015

Uh oh!

luisrudge commented Feb 7, 2015

Uh oh!

XhmikosR commented Feb 9, 2015

Uh oh!

ndelage commented Feb 9, 2015

Uh oh!

ndelage commented Mar 12, 2015

Uh oh!

XhmikosR commented Mar 12, 2015

Uh oh!

ndelage commented Mar 12, 2015

Uh oh!

qkdreyer commented Jun 30, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants