search

bvaughn / react-window

React components for efficiently rendering large lists and tabular data
https://react-window.now.sh/
MIT License
15.54k stars 779 forks source link

fix scrollDirection when direction is RTL #690

Closed DevinFrenze closed 8 months ago

DevinFrenze commented 1 year ago

To determine the scrollDirection in the _onScrollHorizontal method it compares the previous state's scrollOffset with the current scrollLeft value of the target. For browsers whose "RTLOffsetType" is "negative", this results in the scrollDirection being set to "backward".

The scrollOffset state value has been converted to a positive number or zero, and the scrollLeft value of the target (when "RTLOffsetType" is "negative") is a negative number. When comparing prevState.scrollOffset < scrollLeft it is always false because a positive value or zero is never greater than a negative value.

Proposed fix: compare prevState.scrollOffset < scrollOffset because scrollOffset has already been transformed in the same way that prevState.scrollOffset was transformed.

This pull request is intended to address this bug https://github.com/bvaughn/react-window/issues/689.

DevinFrenze commented 1 year ago

@bvaughn sorry to ping you, but if you have a moment to glance at this and let me know if you think it's a reasonable change i would appreciate it. i'm using react-window in a project and would much rather get this change in and use the main repo than be using a fork indefinitely.

and for what it's worth i have tested this some amount and believe that it works as expected.

DevinFrenze commented 8 months ago

@bvaughn i'm requesting your review here. let me know if there's someone else i should reach out to for review.

DevinFrenze commented 8 months ago

I tested it in a project that I'm developing and it works well there, and it doesn't change behavior when using react-window left-to-right. Of course, there may be edge cases I don't know about.

bvaughn commented 8 months ago

That's fair. Okay!

bvaughn commented 8 months ago

@DevinFrenze Please check 1.8.10 (just published) and confirm the fix?

DevinFrenze commented 7 months ago

@bvaughn it appears to be working well ! thanks for merging

bvaughn commented 7 months ago

No problem. Thanks for verifying!