Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

A lot of fixes and improvements #34

Merged
merged 19 commits into from Feb 8, 2013

Conversation

Projects
None yet
7 participants
Contributor

arlm commented Oct 29, 2012

  • Made antiscroll more robust to work between windows (thanks to othree)
  • Added antiscroll wrapper (thanks to othree)
  • Update the scrollbar size when refreshing (thanks to pgherveou)
  • Fixed some performance and cleanup issues (thanks to fontaineshu)
  • Enforced a minimum size on the scrollbars (thanks to fontaineshu)
  • Minor fixes due to all the merges
Contributor

rauchg commented Oct 29, 2012

holy cow

Contributor

rauchg commented Oct 29, 2012

It's gonna take me a bit to review all this :P

Contributor

arlm commented Oct 29, 2012

Fortunately this contemplates all the fixes and improvements on all antiscroll forks that github report. ;-)

One merge to rule'm all !

@arlm this breaks my fix for #30, Math.ceil is used for rounding errors. The merge also erased the same change near line 401.

Owner

arlm replied Oct 31, 2012

Ok, I understand. I have merged this from a performance patch of one of the forks. I really have seen no difference on the final effect I think modern browsers ignore or round it automatically, don't they?

I think this Math.ceil change may be kept out of the patch if it is needed. I really didn't really knew its objective and seemed to be an performance issue.

@arlm It fixed a mousewheel bug in Chrome. Alternatively it can be 1 + this.innerEl.scrollTop + this.pane.el.height().

Owner

arlm replied Oct 31, 2012

Wouldn't this last line of code be more performatic?

@arlm probably, but the difference is not significant at all.

Owner

arlm replied Oct 31, 2012

OK. I'll change it on my branch.

UPDATE: done on 5504c58

tcoulter commented Nov 4, 2012

Whoa, just saw this. Very interested if/when this gets pulled in.

tcoulter commented Nov 4, 2012

It looks like @arlm's request includes mine (LearnBoost#36). If you merge this, you can ignore mine.

@tcoulter tcoulter and 1 other commented on an outdated diff Nov 4, 2012

antiscroll.js
this.padding = undefined == this.options.padding ? 2 : this.options.padding;
this.inner = this.el.find('.antiscroll-inner');
- this.inner.css({
- 'width': '+=' + scrollbarSize()
- , 'height': '+=' + scrollbarSize()
- });
+
+ var cssMap = {};
+ if (this.x) cssMap.width = '+=' + scrollbarSize();
@tcoulter

tcoulter Nov 4, 2012

Shouldn't these be switched i.e., if x add height and if y add width?

@arlm

arlm Nov 5, 2012

Contributor

Yes, thinking about it you are right. You can submit a patch to my branch or wait. I will make a patch to correct it during this week, but I have all days seminar on most of it.

@arlm

arlm Nov 5, 2012

Contributor

I have merged your code on my branch, wich is indeed correct. After my pull-request is accepted I can do another one here with your fixes.

tcoulter commented Nov 5, 2012

Thanks @arlm!

aaronchi commented Feb 8, 2013

👍

rauchg added a commit that referenced this pull request Feb 8, 2013

Merge pull request #34 from arlm/pull-resquest
A lot of fixes and improvements

@rauchg rauchg merged commit 0c1631d into Automattic:master Feb 8, 2013

Contributor

rauchg commented Feb 8, 2013

Fantastic work @arim. Please add yourself and the other contributors involved as a sub-heading "Contributors" under the "Credits" section in the README.

Contributor

arlm commented Feb 14, 2013

I did it on another pull request. Thanks for the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment