RTL Support #497

louiebert · 2019-10-24T19:16:02Z

Adds RTL support by reversing horizontal scrolling and flipping horizontal rendered position of columns.

Description

Flips interpretations of scroll/keyboard events and flips element positions on render. This approach keeps the app state unchanged, so any callback props users would still receive the same relative values.

Motivation and Context

Some languages read from right to left, so this enables a native support for that use case.
Fixes #209

How Has This Been Tested?

I walked through and manually tested every page on the examples from the locally hosted site, ensuring that every feature not only behaved, but looked the way that it should upon the direction being switched.
Additionally, I wrote some unit tests to validate that the state values and DOM element positions behave as expected during horizontal scrolling.

Screenshots:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.

Aria selected and row attributes

…m rowSettings

…ent dir

…render

src/FixedDataTableCell.js

wcjordan

Any idea what the backwards compatibility of this change will be? Will behavior be changed for users in a RTL locale?

src/vendor_upstream/dom/translateDOMPositionXY.js

louiebert · 2019-10-30T20:25:39Z

Any idea what the backwards compatibility of this change will be? Will behavior be changed for users in a RTL locale?

From what I could tell from the issue this is tied to, users in an RTL locale are not being supported currently. Before this change, they'd need custom styling and position overrides.

I don't think this would break backward compatibility, since it seems that there was no support to start with.

louiebert · 2019-11-06T20:27:25Z

@pradeepnschrodinger @novakps @somavara @mycrobe would you mind reviewing this when you get some time?

pradeepnschrodinger · 2019-11-11T20:22:42Z

@pradeepnschrodinger @novakps @somavara @mycrobe would you mind reviewing this when you get some time?

Sorry for the delays, we'll review it soon.
This looks good at first glance, and thanks a lot for adding tests.

src/stubs/Locale.js

src/css/style/fixedDataTableCell.css

src/vendor_upstream/dom/ReactWheelHandler.js

src/FixedDataTableCellGroup.js

pradeepnschrodinger · 2019-11-18T09:37:31Z

src/FixedDataTableTranslateDOMPosition.js

@@ -20,9 +22,18 @@ function FixedDataTableTranslateDOMPosition(/*object*/ style, /*number*/ x, /*nu
    style.left = x + 'px';
    style.top = y + 'px';
  } else {
+    if (BrowserSupportCore.hasCSSTransforms() && Locale.isRTL()) {


what's the use of BrowserSupportCore.hasCSSTransforms ?

Inside translateDOMPositionXY.js, when BrowserSupportCore.hasCSSTransforms is false, the result of the translate is an inline style change of left and top, whereas if that condition is true, a translate3d is performed.

Basically, it created this situation where the x value should be flipped when BrowserSupportCore.hasCSSTransforms is true, but should not be when it's false (since the value should remain unchanged and be moved from style.left to style.right)

src/FixedDataTableTranslateDOMPosition.js

pradeepnschrodinger · 2019-11-18T09:43:03Z

src/FixedDataTableTranslateDOMPosition.js

+    style.right = style.left;
+    style.left = 'auto';


Is this needed? I thought flipping the x value from the condition above will mean that translateDOMPositionXY will also flip the right and left values.

Depending on browser support, either translate3d is used or the style.left is changed. In either case, it would be safe to flip the style.left and style.right values

pradeepnschrodinger · 2019-11-18T13:31:18Z

I strongly prefer RTL controlled through a table prop rather than relying on the html document's property.

louiebert · 2019-11-18T20:14:06Z

Thank you for the review comments. Currently revising my PR to account for those.

I strongly prefer RTL controlled through a table prop rather than relying on the html document's property.

In the meantime, I figure we could discuss a bit more on how to implement the RTL detection. I chose to do it by looking at the dir attribute based on some common practices I found and some documentation (here and here).

Why would a prop be a better way of handling this?

pradeepnschrodinger · 2019-11-19T14:17:42Z

In the meantime, I figure we could discuss a bit more on how to implement the RTL detection. I chose to do it by looking at the dir attribute based on some common practices I found and some documentation (here and here).

Why would a prop be a better way of handling this?

Prop fits our table better as the table is a react component itself.
You won't have any trouble with detection since the table will be rendered along with the prop change. Also this will be applied just to the table, rather than being grouped with the entire html. This means we have finer control.

pradeepnschrodinger

Looking good, just a few minor comments before we proceed on merging.

src/FixedDataTableCell.js

src/FixedDataTableColumnReorderHandle.js

…-table-2 into rtl-support

pradeepnschrodinger · 2019-11-27T16:04:37Z

@wcjordan , this looks go to be merged. I tested out the examples and also tried it on LD, and works fine.

There's just a minor CSS issue in RTL mode, but I think we can address it later through as a separate issue. It doesn't affect existing users anyway.

Shall I proceed on merging?

wcjordan · 2019-11-27T17:59:46Z

Great, that sounds good to me. Feel free to go ahead.

pradeepnschrodinger · 2019-11-28T16:12:45Z

@louiebert , released with v1.0.2

louiebert · 2019-11-28T16:46:19Z

Fantastic!! Thank you guys for the thorough review!

loberton and others added 12 commits October 14, 2019 11:14

Adding selected props

f2bd755

RTL appears to work, now to write tests

80f5a9e

Made getIsRowSelected more generic and moved to rowSettings

3e10119

Merge pull request #1 from louiebert/aria-selected

fdc4b11

Aria selected and row attributes

Updated FDTBufferedRows to correctly retrieve rowAttributesGetter fro…

cc8063e

…m rowSettings

Added missing import to ColumnReorderHandle

af9f8e8

Created a more generic attributes method for the grid

a1c2d4a

Added null check to gridAttributesGetter prop

52976c0

Made styles a little more specific and made Locale dependent on docum…

114a7fb

…ent dir

Slimmed down changes to make sign reversals only happen at input and …

0cff5d7

…render

Feature is now tested and is based off of the body's dir attribute

a08d105

Removed rtl dir on the document that was inserted for testing

f16ef39

Tanner-MS reviewed Oct 25, 2019

View reviewed changes

src/FixedDataTableCell.js Outdated Show resolved Hide resolved

Removed package-lock.json and adjusted an if condition

f69c225

pradeepnschrodinger requested review from pradeepnschrodinger, wcjordan and somavara October 30, 2019 00:26

pradeepnschrodinger added the enhancement label Oct 30, 2019

wcjordan requested review from novakps and mycrobe October 30, 2019 16:55

wcjordan reviewed Oct 30, 2019

View reviewed changes

src/vendor_upstream/dom/translateDOMPositionXY.js Show resolved Hide resolved

Moved RTL translations from vendor_upstream to FDTTranslateDOMPosition

cd1e783

pradeepnschrodinger requested changes Nov 18, 2019

View reviewed changes

Made conditional changes and renamed DIR_SIGN based on review feedback

b5d35ac

louiebert added 3 commits November 20, 2019 11:21

Shifted RTL mode to be set by table prop

a2d58a0

Fixed merge conflicts

93282e4

Merge branch 'master' into rtl-support

659fa43

pradeepnschrodinger approved these changes Nov 26, 2019

View reviewed changes

src/FixedDataTableCell.js Outdated Show resolved Hide resolved

src/FixedDataTableColumnReorderHandle.js Outdated Show resolved Hide resolved

louiebert added 3 commits November 26, 2019 10:38

Addressed review comments regarding DIR_SIGN member variables

b9ea02a

Merge branch 'rtl-support' of https://github.com/louiebert/fixed-data…

0e6a354

…-table-2 into rtl-support

Moved a variable declaration to be in correct scope

54e6059

pradeepnschrodinger merged commit bb27bd2 into schrodinger:master Nov 28, 2019

pradeepnschrodinger mentioned this pull request Nov 28, 2019

RTL data-table #209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTL Support #497

RTL Support #497

louiebert commented Oct 24, 2019 •

edited

Loading

wcjordan left a comment

louiebert commented Oct 30, 2019

louiebert commented Nov 6, 2019

pradeepnschrodinger commented Nov 11, 2019

pradeepnschrodinger Nov 18, 2019

louiebert Nov 18, 2019

pradeepnschrodinger Nov 18, 2019

louiebert Nov 18, 2019

pradeepnschrodinger commented Nov 18, 2019

louiebert commented Nov 18, 2019

pradeepnschrodinger commented Nov 19, 2019

pradeepnschrodinger left a comment

pradeepnschrodinger commented Nov 27, 2019

wcjordan commented Nov 27, 2019

pradeepnschrodinger commented Nov 28, 2019

louiebert commented Nov 28, 2019

RTL Support #497

RTL Support #497

Conversation

louiebert commented Oct 24, 2019 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots:

Types of changes

Checklist:

wcjordan left a comment

Choose a reason for hiding this comment

louiebert commented Oct 30, 2019

louiebert commented Nov 6, 2019

pradeepnschrodinger commented Nov 11, 2019

pradeepnschrodinger Nov 18, 2019

Choose a reason for hiding this comment

louiebert Nov 18, 2019

Choose a reason for hiding this comment

pradeepnschrodinger Nov 18, 2019

Choose a reason for hiding this comment

louiebert Nov 18, 2019

Choose a reason for hiding this comment

pradeepnschrodinger commented Nov 18, 2019

louiebert commented Nov 18, 2019

pradeepnschrodinger commented Nov 19, 2019

pradeepnschrodinger left a comment

Choose a reason for hiding this comment

pradeepnschrodinger commented Nov 27, 2019

wcjordan commented Nov 27, 2019

pradeepnschrodinger commented Nov 28, 2019

louiebert commented Nov 28, 2019

louiebert commented Oct 24, 2019 •

edited

Loading