cameronmcefee / plax

JQuery powered parallaxing
http://www.cameronmcefee.com/plax
MIT License
2.28k stars 206 forks source link

Add option for restoring the original positions upon disable. #40

Closed magebarf closed 11 years ago

magebarf commented 11 years ago

This is quite useful when your page has multiple sections where you won't be seeing all of them at the same time.

In this case you wouldn't want the entire set of plax layers to be enabled all the time, and as such the clearLayers option is great.

However, moving back and forth from different sections of the page, there's a risk the objects aren't in their original position when clearing the layers, and when moving back and re-enabling plax on them, they would get a new initial position, which would be stacking up for each time you move back and forth to the section.

New option simply makes sure that each layer's original position is restored upon disable, normally combined with clearLayers, but there are possible uses without clearing the layers as well.

cameronmcefee commented 11 years ago

Nice, I dig this. If you don't mind updating the one part in the documentation, I'm down to merge this.

magebarf commented 11 years ago

Yup, it should be a quite straight forward option, not needing too much of an explanation.

My ambiguous description was intended to convey the message that it was useful when you were using a combination of Plax and Turn.js, as you might want to enable parallax scrolling for elements only on the page you are currently watching, and disabling it in between.

Another thing that I started to wonder a bit about today, is why both the $(this).position() and this.offset[Left[Top] are used in the initialization of the non-background layers, and if possibly the layer.origin variables should be set to the $(this).position values instead.

As far as I can tell though, after refreshing my memory of the jQuery documentation is that they should be the same values. Reading up a bit on the jQuery code, the only potential difference I can see is that the .position() values deducts the margin, which I'm unsure if the offset values handles?

Also, I'm unsure if Ender differs in this aspect?

Please pitch your thoughts on this... It works just fine in my use cases with these commits in place, but I might be missing something outside of my scenario...

magebarf commented 11 years ago

And to fill in after a bit more research on Ender, and more specifically Jeesh/Bonzo (which I assume is the parts of Ender meant):

It does actually not have any .position() function, and thus I believe that commit 918b527c6984e4e97ecbeb36d111dc341a273298 broke ender/Jeesh compatibility (modifying the examples att current head to load Jeesh instead of jQuery logs "Uncaught TypeError: Object [object Array] has no method 'position'" to the console).

cameronmcefee commented 11 years ago

I originally accepted the Ender support without really considering what that would mean (I was very new to open-source at the time) as far as supporting it myself. Having never used Ender before or since then, it's more of a hurdle for maintaining the code on my end. If the Ender support is actually broken, my vote would be to just remove it and not sweat it.

Another thing that I started to wonder a bit about today, is why both the $(this).position() and this.offset[Left[Top] are used in the initialization of the non-background layers, and if possibly the layer.origin variables should be set to the $(this).position values instead.

It's entirely possible this was done arbitrarily. When I originally wrote plax I was pretty green as far as my javascript chops. It may have just been a terrible decision on my part. One of these days I may just rewrite the things, but for now I've been letting stuff sit as is until I or someone else can really focus on it.

magebarf commented 11 years ago

I'll make sure to put it up as a separate issue, as its not really related to this pull request. I can prepare a small patch to remove the usage of .position instead of the offset properties, and it'll be back on track again, as that as far as I can tell is the only part incompatible with the default Ender package (Jeesh).

cameronmcefee commented 11 years ago

:fire: thanks.