Skip to content

Feature: RTL #2803

New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 54 commits into from
Feb 10, 2022
Merged

Feature: RTL #2803

merged 54 commits into from
Feb 10, 2022

Conversation

amanmahajan7
Copy link
Contributor

@amanmahajan7 amanmahajan7 commented Jan 31, 2022

Fixes #1742

  • Add a new direction prop
  • Use logical properties instead of physical. It is supported in all the browsers. Rest is handled by CSS grid
  • Flip ArrowLeft/ArrowRight keys for RTL
  • Use :dir pseudo-class to tweak styles like box shadows

Issues:

  • Chrome has a bug with frozen columns
  • Chrome does not support :dir pseudo-class but this only breaks transforms and other minor styles
  • Chrome does not support float: inline-end

This PR only adds support for RTL and not for writing modes

Example of RTL
https://www.ag-grid.com/javascript-data-grid/rtl/
https://www.telerik.com/kendo-react-ui/components/grid/globalization/#toc-right-to-left-support

@amanmahajan7 amanmahajan7 self-assigned this Jan 31, 2022
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #2803 (4929122) into main (9aa9cf6) will increase coverage by 0.00%.
The diff coverage is 92.00%.

❗ Current head 4929122 differs from pull request most recent head 5600176. Consider uploading reports for the commit 5600176 to get more accurate results

@@           Coverage Diff           @@
##             main    #2803   +/-   ##
=======================================
  Coverage   96.16%   96.17%           
=======================================
  Files          37       37           
  Lines        1227     1230    +3     
  Branches      383      387    +4     
=======================================
+ Hits         1180     1183    +3     
  Misses         47       47           
Impacted Files Coverage Δ
src/DragHandle.tsx 100.00% <ø> (ø)
src/GroupRow.tsx 100.00% <ø> (ø)
src/HeaderRow.tsx 100.00% <ø> (ø)
src/SummaryCell.tsx 100.00% <ø> (ø)
src/SummaryRow.tsx 100.00% <ø> (ø)
src/editors/TextEditor.tsx 100.00% <ø> (ø)
src/formatters/CheckboxFormatter.tsx 100.00% <ø> (ø)
src/formatters/ToggleGroupFormatter.tsx 100.00% <ø> (ø)
src/style/cell.ts 100.00% <ø> (ø)
src/style/core.ts 100.00% <ø> (ø)
... and 4 more

@@ -583,7 +590,7 @@ function DataGrid<R, SR, K extends Key>(
function handleScroll(event: React.UIEvent<HTMLDivElement>) {
const { scrollTop, scrollLeft } = event.currentTarget;
setScrollTop(scrollTop);
setScrollLeft(scrollLeft);
setScrollLeft(abs(scrollLeft));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is negative in RTL

@amanmahajan7 amanmahajan7 marked this pull request as ready for review January 31, 2022 16:29
@amanmahajan7 amanmahajan7 added the V8 V8 Features label Jan 31, 2022
Copy link
Contributor

@nstepien nstepien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's list RTL support in the README.
Do we have any links to open browser issues? Might be good to list them in the RTL demo page.

src/DataGrid.tsx Outdated
@@ -368,6 +371,10 @@ function DataGrid<R, SR, K extends Key>(
* effects
*/
useLayoutEffect(() => {
if (isRtlRef.current === undefined) {
isRtlRef.current = isRtlDirection(gridRef.current!);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is executed on each render, it assumes that changes to the direction will trigger a re-render of RDG, which might not necessarily be true (memoized parent components, just css change, ...), so I'm not sure it's super safe.
We could have an optional direction prop, and fall back to useLayoutEffect if it's not specified.

I wonder if the ResizeObserver is triggered when the direction changes 🤔

@@ -53,3 +53,7 @@ export function getCellClassname<R, SR>(
...extraClasses
);
}

export function isRtlDirection(element: Element) {
return getComputedStyle(element).direction === 'rtl';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle

The returned style is a live CSSStyleDeclaration object, which updates automatically when the element's styles are changed.

Wonder if it's worth only getting that object once.
Could use a WeakMap for the cache.

Copy link
Contributor

@nstepien nstepien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's document the direction prop in the readme.
some new test for keyboard nav would be good too

expect(getGrid()).toHaveAttribute('dir', 'rtl');
});

test('should not change the left and right arrow behavior for right to left languages', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be moved to test/keyboardNavigation.test.tsx IMO.

nstepien
nstepien previously approved these changes Feb 10, 2022
Copy link
Contributor

@nstepien nstepien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@amanmahajan7 amanmahajan7 merged commit c0e4a63 into main Feb 10, 2022
@amanmahajan7 amanmahajan7 deleted the am-rtl branch February 10, 2022 15:59
keriat pushed a commit to superform-xyz/react-data-grid that referenced this pull request Jan 10, 2024
## [5.1.0](v5.0.4...5.1.0) (2024-01-10)

### Features

* Add DataGridComponentsProvider component and DataGrid.components prop ([adazzle#2754](https://github.com/superform-xyz/react-data-grid/issues/2754)) ([f864291](f864291))
* Add support for flex column width ([adazzle#2839](https://github.com/superform-xyz/react-data-grid/issues/2839)) ([4a136ad](4a136ad))
* expose scroll event ([adazzle#2011](https://github.com/superform-xyz/react-data-grid/issues/2011)) ([7614c8e](7614c8e))
* RTL ([adazzle#2803](https://github.com/superform-xyz/react-data-grid/issues/2803)) ([c0e4a63](c0e4a63))
* semantic release installed and configured ([e71bfe6](e71bfe6))

### Bug Fixes

* drag jumps to the right due to the failed merge ([adazzle#1564](https://github.com/superform-xyz/react-data-grid/issues/1564)) ([7033a1b](7033a1b))
* isSelectedCellEditable row getter idx ([adazzle#1743](https://github.com/superform-xyz/react-data-grid/issues/1743)) ([018f137](018f137))
* package-lock.json added to version control ([df39113](df39113))

### Reverts

* Revert "Dependabot: set versioning-strategy to increase (adazzle#2479)" (adazzle#2511) ([0bedc81](0bedc81))
keriat pushed a commit to superform-xyz/react-data-grid that referenced this pull request Jan 10, 2024
## [5.1.0](v5.0.4...5.1.0) (2024-01-10)

### Features

* Add DataGridComponentsProvider component and DataGrid.components prop ([adazzle#2754](https://github.com/superform-xyz/react-data-grid/issues/2754)) ([f864291](f864291))
* Add support for flex column width ([adazzle#2839](https://github.com/superform-xyz/react-data-grid/issues/2839)) ([4a136ad](4a136ad))
* expose scroll event ([adazzle#2011](https://github.com/superform-xyz/react-data-grid/issues/2011)) ([7614c8e](7614c8e))
* RTL ([adazzle#2803](https://github.com/superform-xyz/react-data-grid/issues/2803)) ([c0e4a63](c0e4a63))
* semantic release installed and configured ([e71bfe6](e71bfe6))

### Bug Fixes

* drag jumps to the right due to the failed merge ([adazzle#1564](https://github.com/superform-xyz/react-data-grid/issues/1564)) ([7033a1b](7033a1b))
* isSelectedCellEditable row getter idx ([adazzle#1743](https://github.com/superform-xyz/react-data-grid/issues/1743)) ([018f137](018f137))
* package-lock.json added to version control ([df39113](df39113))

### Reverts

* Revert "Dependabot: set versioning-strategy to increase (adazzle#2479)" (adazzle#2511) ([0bedc81](0bedc81))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
V8 V8 Features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

direction:RTL is not working
2 participants