Scroller class not working #1012

Closed
wnkz opened this Issue Aug 18, 2011 · 5 comments

3 participants

@wnkz

I have found that the Scroller class was not working properly.

scroll: function(){
  // ...
  pos = this.element != this.docBody ? this.element.getOffsets() : {x: 0, y:0},
  // ...
}

When instantiating class like this : new Scroller(window); ; the previous method calls getOffsets of window and it's not defined.

There is more than one way to fix this so I will let you decide.
Either you can implement getOffsets in window and document like this :

[Document, Window].invoke('implement', {
  // ...
  getOffsets: function(){
    return {x: 0, y: 0};
  },
  // ...
}

or you can fix the Scroller class the way you want.

It is your call, you probably know better than me.

@arian
MooTools member

why not use

new Scroller(document.body).

or otherwise we could fix it by using:

this.element != this.docBody && this.element != window ? this.element.getOffsets() : {x: 0, y: 0}
@wnkz

I got to admit I simply did not though of it ...

Anyway I still think it's a problem, I have been influenced by an exemple in the documentation that is using window (http://mootools.net/docs/more/Interface/Scroller) :

var myScroller = new Scroller(window, {
    area: Math.round(window.getWidth() / 5)
});

(function(){
    this.stop();
    this.start();
}).periodical(1000, myScroller);

Fix it the way you want, I am confident ; or maybe you want me to make a pull request ?

@arian
MooTools member

a pull request with:

        var element = this.element,
            size = element.getSize(),
            scroll = element.getScroll(),
            pos = element != this.docBody && element != window ? element.getOffsets() : {x: 0, y:0},
            scrollSize = element.getScrollSize(),
            change = {x: 0, y: 0},
            top = this.options.area.top || this.options.area,
            bottom = this.options.area.bottom || this.options.area;

would certainly be awesome :)

@wnkz

There you are #1013 ;)

@cdekok

This is still a problem also the documentation explains it works with window which it does not!

@SergioCrisostomo SergioCrisostomo added a commit to SergioCrisostomo/mootools-more that referenced this issue Jun 20, 2014
@SergioCrisostomo SergioCrisostomo Correct docs
Docs were giving example with `window`as element. That breaks the code
as described in #1012

This commit changes the example to something one can copy/paste and see
it working "out of the box":  http://jsfiddle.net/v4yJD/

It changes also the first parameter info, from element to mixed, since
code source uses `this.element = document.id(element);`
c6043b3
@SergioCrisostomo SergioCrisostomo added a commit to SergioCrisostomo/mootools-more that referenced this issue Jun 20, 2014
@SergioCrisostomo SergioCrisostomo Docs correction
Docs were giving example with `window`as element. That breaks the code
as described in #1012

This commit changes the example to something one can copy/paste and see
it working "out of the box":  http://jsfiddle.net/v4yJD/

It changes also the first parameter info, from element to mixed, since
code source uses `this.element = document.id(element);`
a731b95
@arian arian closed this in #1269 Jun 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment