Add rounding to horizontal scroll #262
Conversation
sergek
force-pushed
the
master
branch
2 times, most recently
from
9376be7
to
a95c02c
Compare
| @@ -1426,10 +1426,13 @@ var FixedDataTable = createReactClass({ | |||
| if (!this._isScrolling) { | |||
| this._didScrollStart(); | |||
| } | |||
|
|
|||
| var roundedScrollPos = Math.round(scrollPos); | |||
There was a problem hiding this comment.
Awesome, thanks for digging ito this more. Do we need to do the same thing when we set scrollX in _onScroll? Is it worth doing for scrollY as well?
There was a problem hiding this comment.
For _onScroll there is no need as it doesn't directly change dom.
And there is rounding in scrollY already in place.
There was a problem hiding this comment.
Could you elaborate on your comment wrt _onScroll? Both this method and that one call setState with a new value for scrollX from my read. Does the comment refer to it not being triggered by a scrollbar drag event? How does that come into play?
There was a problem hiding this comment.
After reading it again I think you are absolutely right - there is the same thing going on in both methods.
I've updated the pr, but right now I can't test if this change has any side effects on complex table layouts.
Will check it out on Monday.
There was a problem hiding this comment.
This should be safe. There's a small issue that the scrollbar position will be out of sync from the grid scroll position by a fraction of a pixel. Since we sync the scrollbar & grid with absolute positions rather than deltas we shouldn't get any drift though.
I'll give this a once over to ensure there's no weird performance issue if they get out of sync and try setting each other.
There was a problem hiding this comment.
Checked and this doesn't cause render loops. If scrollbar is a fraction, then the main grid handles and re-renders it as a non fraction and we're back on track.
I think this is good to merge. any final concerns from your end before I merge?
There was a problem hiding this comment.
It works well in my environment.
I hope this is a relatively temporary measure and the gpu text rendering will improve in browsers over time.
| @@ -1426,10 +1426,13 @@ var FixedDataTable = createReactClass({ | |||
| if (!this._isScrolling) { | |||
| this._didScrollStart(); | |||
| } | |||
|
|
|||
| var roundedScrollPos = Math.round(scrollPos); | |||
There was a problem hiding this comment.
Checked and this doesn't cause render loops. If scrollbar is a fraction, then the main grid handles and re-renders it as a non fraction and we're back on track.
I think this is good to merge. any final concerns from your end before I merge?
|
Released w/ 0.8.7 |
Description
Add rounding to horizontal scroll to avoid content blurring (in case translate3d is used).
Motivation and Context
Fixes issue:
#254
How Has This Been Tested?
Manually tested in Chrome, Safairi, FF of latest release versions.
Screenshots (if appropriate):
Unrounded
Rounded
Types of changes
Checklist: