feat: rtl support for components [LIBS-525] #1448

tomzemp · 2024-02-07T14:15:09Z

Description

This PR adds RTL support to our existing components. This involves:

updates to use logical properties (e.g. padding-inline-start instead of padding-left)
some additional changes (e.g. icon orientation) handled with css pseudoclasses (:dir(rtl)) (e.g. to transform arrows in transfer component) to make sense with dir=rtl
some direct remappings from left to right (and vice versa) where this is provided (e.g. in Tables)
some remapping of cases where we have allowed for non-logical property css (e.g. remapping margin in divider if dir=rtl)

Note on approach: I've made these changes manually rather than using a code mode, in part because the code mods I saw seemed to mostly be focused on css files where we have styling as jsx for most components. Also, making manual changes allowed more control over the actual changes (see points below). Finally, there were a number of issues that would not be addressed by updating logical properties.

Note on logical properties: For the most part I have only updated to logical properties to convert left/right to inline. This simplifies things a bit as some existing shorthand notation does not need updating (e.g. padding: 4px 8px has equal values on left/right, so doesn't need to be converted to logical property). I opted for mostly minimally intrusive changes, so I left valid values that did not cause issues.

I didn't think it was worth it to convert top/bottom to block logical properties because the changes needed to support tb text is much more comprehensive (and involves updating css with changes like width to inline-size and also setting writing-mode (which we're not doing)). There's only one major script (traditional Mongolian) that is written exclusively TB, and DHIS2 is only used by NGOs in Mongolia; also the Mongolian language can be written in Cyrillic (which seems to be the more common script). So, in general I've just kept top/bottom references and only switched to block in a couple cases where I was already breaking out something like margin: 1px 5px 10px 2px.

Known issues

Switch component does not display checkmark in RTL rotation

LTR:

RTL:

This is because I've applied a transformation on the entire switch icon, which would also rotate the checkmark (undesired), so I have defaulted to hiding it until we make more comprehensive update to Switch

Calendar and CalendarInput were not updated because of errors preventing their display, but I believe these were already RTL compliant?

Checklist

API docs are generated
I've generally not updated API docs because I haven't changed the API. I think we want people to generally assume that the component will display correctly in RTL? I have updated the chip component documentation because I have added marginInlineStart and marginInlineEnd to complement marginLeft and marginRight props we already had
Tests were added
We discussed this before in context of previous updates and decided that adding tests for RTL was not necessary (at least for logical properties updates). If we think there should be tests for non-logical-property changes, I could add some?
Storybook demos were added
I've added a demo (generally called RTL) for components that will look different in RTL (i.e. most of them). I've added a story even in a few cases where there are no actual updates to the component itself (e.g. Linear Loader) just to illustrate how things will look.

Note because of the portal, I sometimes need to use useEffect within the storybook demos to set the document direction (with app-platform the portal direction should also be set, so this is strictly a step for making the storybook demos behave appropriately)

Screenshots

There are a lot of changes, so probably best to run the storybook locally and review the RTL stories.

dhis2-bot · 2024-02-07T14:19:37Z

🚀 Deployed on https://pr-1448--dhis2-ui.netlify.app

tomzemp · 2024-02-07T14:25:03Z

components/divider/src/divider.js

-        `}</style>
-    </div>
-)
+const flipMargin = (margin) => {


I'm not totally sure if we want to try to remap margin provided as a prop, but this seemed probably more helpful to attempt?

this is the best we can do for now .. I am collecting some breaking changes we should do for the library, and this should be one of them at some point (dropping the margin prop)

tomzemp · 2024-02-07T14:26:57Z

components/chip/API.md

@@ -22,8 +22,10 @@ import { Chip } from '@dhis2-ui/chip'
 |dragging|boolean||||
 |icon|element||||
 |marginBottom|number|`4`||`margin-bottom` value, applied in `px`|
-|marginLeft|number|`4`||`margin-left` value, applied in `px`|
-|marginRight|number|`4`||`margin-right` value, applied in `px`|
+|marginInlineStart|number|`4`||`margin-inline-start` value, applied in `px`|


I thought we would want to leave the existing marginLeft and marginRight properties and not treat them as equivalent to inline-start inline-end, but that going forward it would be better to also provide for people to enter the margin in a logical-property-compliant manner (hence addition of marginInlineStart and marginInlineEnd here)

shouldn't this include an update to the TypeScript definitions?

tomzemp · 2024-02-07T14:29:33Z

components/popper/src/popper.js

@@ -32,7 +42,10 @@ const Popper = ({
    const { styles, attributes } = usePopper(referenceElement, popperElement, {
        strategy,
        onFirstUpdate,
-        placement,
+        placement:
+            document.documentElement.dir === 'rtl'


I don't know what the ideal is here? The popper library that we're using says that it handles RTL, but it seems to provide placement only with physical properties. I assume we want to switch these, so I have done so based on the document.documentElement direction that is being set by app-platform, but I'm not sure if there's a reason why we might want to strictly interpret the placement as a physical property?

again, not ideal - I think the main reason we wouldn't want to do the flipping, is that people who are actually writing apps for an RTL-language would set the placement correctly, and then we will go ahead and flip it.

But we can assume for backwards-compatibility that any implementation out there are LTR, and keep this logic. In the future, maybe the flipping can be controlled with a different prop

tomzemp · 2024-02-07T14:31:17Z

components/table/src/data-table/table-elements/table-data-cell/table-data-cell.js

@@ -5,6 +5,12 @@ import PropTypes from 'prop-types'
 import React, { forwardRef } from 'react'
 import styles from './table-data-cell.styles.js'

+const rtlCorrespondingAlignments = {


here it also seemed like we would want to assume that the physical properties we have on this component for alignment should actually behave as logical properties. Though I guess we could also add new options like inline-start, inline-end for alignment?

cypress · 2024-02-07T14:35:11Z

Passing run #3210 ↗︎

0	584	0	0	0

Details:

fix: pr clean up
Project: ui	Commit: `bd5f696b1c`
Status: Passed	Duration: 06:46 💡
Started: Feb 8, 2024 12:14 PM	Ended: Feb 8, 2024 12:21 PM

Review all test suite changes for PR #1448 ↗︎

tomzemp · 2024-02-08T09:26:01Z

components/header-bar/src/notification-icon.js

@@ -12,7 +12,7 @@ function icon(kind) {
 }

 export const NotificationIcon = ({ count, href, kind, dataTestId }) => (
-    <a href={href} className={kind} data-test={dataTestId}>
+    <a dir="ltr" href={href} className={kind} data-test={dataTestId}>


this is set to ltr so that the count bubbles appear on the right of the icons. I think the number + icon effectively behaves as an icon unto itself, and moving the number to the left dir=rtl, makes the icon harder to interpret (particularly the messages one)

kabaros

great stuff @tomzemp 💯 - some minor comments, but let's merge it asap (at least to alpha) and see how it works in an app

kabaros · 2024-02-07T16:05:23Z

components/chip/API.md

@@ -22,8 +22,10 @@ import { Chip } from '@dhis2-ui/chip'
 |dragging|boolean||||
 |icon|element||||
 |marginBottom|number|`4`||`margin-bottom` value, applied in `px`|
-|marginLeft|number|`4`||`margin-left` value, applied in `px`|
-|marginRight|number|`4`||`margin-right` value, applied in `px`|
+|marginInlineStart|number|`4`||`margin-inline-start` value, applied in `px`|


shouldn't this include an update to the TypeScript definitions?

kabaros · 2024-02-08T09:53:27Z

components/divider/src/divider.js

-        `}</style>
-    </div>
-)
+const flipMargin = (margin) => {


this is the best we can do for now .. I am collecting some breaking changes we should do for the library, and this should be one of them at some point (dropping the margin prop)

kabaros · 2024-02-08T09:56:30Z

components/divider/src/divider.js

-    </div>
-)
+const flipMargin = (margin) => {
+    const splitMargin = margin.split(' ')


I guess this logic would break if you had multiple spaces between the marging values? .split(/\s+/) would be safer, I'd say

kabaros · 2024-02-08T10:05:23Z

components/popper/src/popper.js

@@ -32,7 +42,10 @@ const Popper = ({
    const { styles, attributes } = usePopper(referenceElement, popperElement, {
        strategy,
        onFirstUpdate,
-        placement,
+        placement:
+            document.documentElement.dir === 'rtl'


again, not ideal - I think the main reason we wouldn't want to do the flipping, is that people who are actually writing apps for an RTL-language would set the placement correctly, and then we will go ahead and flip it.

But we can assume for backwards-compatibility that any implementation out there are LTR, and keep this logic. In the future, maybe the flipping can be controlled with a different prop

kabaros · 2024-02-08T10:09:27Z

components/table/src/data-table/table-elements/table-data-cell/table-data-cell.js

-        </td>
-    )
+    ) => {
+        const rtlAlign = rtlCorrespondingAlignments[align]


shouldn't be the case, but let's fallback to what they pass if no match const rtlAlign = rtlCorrespondingAlignments[align] ?? align

# [9.3.0-alpha.1](v9.2.0...v9.3.0-alpha.1) (2024-02-08) ### Features * rtl support for components [LIBS-525] ([#1448](#1448)) ([d8b5fec](d8b5fec))

dhis2-bot · 2024-02-08T12:47:20Z

🎉 This PR is included in version 9.3.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [9.4.0-alpha.1](v9.3.0...v9.4.0-alpha.1) (2024-02-14) ### Bug Fixes * rtl cleanup ([#1450](#1450)) ([5f6761f](5f6761f)) ### Features * rtl support for components [LIBS-525] ([#1448](#1448)) ([d8b5fec](d8b5fec))

dhis2-bot · 2024-02-14T12:45:47Z

🎉 This PR is included in version 9.4.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

tomzemp added 25 commits February 2, 2024 16:49

fix: rtl support for header bar

70959b1

feat: rtl for alerts

014076c

feat: rtl for buttons

c412633

feat: rtl for checkbox

03963c5

feat: rtl for chip

b2f53f4

feat: rtl for loader

f9e2543

feat: rtl for table

08d1011

feat: rtl for divider

85b71d6

feat: rtl for fileInput

10aeb53

feat: rtl for menu

491b485

feat: rtl for input

6f45f8c

feat: rtl for modal

f269b2f

feat: rtl for orgUnitTree / nodes

4488d59

feat: rtl for pagination

3f12f23

feat: rtl for radio

cd0de17

feat: rtl for required

ad9bfa4

feat: rtl for select

6063311

feat: rtl for selector bar

2c69abd

feat: rtl for sharing dialog

bb8329a

feat: rtl for stacked table

a25e216

feat: rtl for tab

300ba2e

feat: rtl for tag

1664cda

feat: rtl story for textarea

b2844c4

feat: rtl for transfer

3e2e94d

feat: rtl for switch (start)

a9e69c2

tomzemp requested a review from a team as a code owner February 7, 2024 14:15

dhis2-bot temporarily deployed to netlify February 7, 2024 14:19 Inactive

feat: rtl for tooltip, popper; cleanup

d16afce

tomzemp force-pushed the rtl-support branch from 54d97c8 to d16afce Compare February 7, 2024 14:21

tomzemp commented Feb 7, 2024

View reviewed changes

dhis2-bot temporarily deployed to netlify February 7, 2024 14:25 Inactive

tomzemp commented Feb 7, 2024

View reviewed changes

tomzemp changed the base branch from master to alpha February 8, 2024 09:23

tomzemp commented Feb 8, 2024

View reviewed changes

kabaros approved these changes Feb 8, 2024

View reviewed changes

fix: pr clean up

bd5f696

dhis2-bot temporarily deployed to netlify February 8, 2024 12:11 Inactive

tomzemp merged commit d8b5fec into alpha Feb 8, 2024
13 checks passed

tomzemp deleted the rtl-support branch February 8, 2024 12:29

dhis2-bot added a commit that referenced this pull request Feb 8, 2024

chore(release): cut 9.3.0-alpha.1 [skip release]

2668bbf

# [9.3.0-alpha.1](v9.2.0...v9.3.0-alpha.1) (2024-02-08) ### Features * rtl support for components [LIBS-525] ([#1448](#1448)) ([d8b5fec](d8b5fec))

dhis2-bot added the released on @alpha label Feb 8, 2024

tomzemp mentioned this pull request Feb 13, 2024

feat: add rtl support for ui components #1452

Merged

6 tasks

This was referenced May 31, 2024

feat: improve support for right-to-left languages in Dashboard app dhis2/dashboard-app#2985

Open

feat: improve support for right-to-left languages in Data Visualiser dhis2/data-visualizer-app#3073

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rtl support for components [LIBS-525] #1448

feat: rtl support for components [LIBS-525] #1448

tomzemp commented Feb 7, 2024 •

edited

Loading

dhis2-bot commented Feb 7, 2024 •

edited

Loading

tomzemp Feb 7, 2024

kabaros Feb 8, 2024

tomzemp Feb 7, 2024

kabaros Feb 7, 2024

tomzemp Feb 7, 2024

kabaros Feb 8, 2024

tomzemp Feb 7, 2024

cypress bot commented Feb 7, 2024 •

edited

Loading

tomzemp Feb 8, 2024

kabaros left a comment

kabaros Feb 7, 2024

kabaros Feb 8, 2024

kabaros Feb 8, 2024

kabaros Feb 8, 2024

kabaros Feb 8, 2024

dhis2-bot commented Feb 8, 2024

dhis2-bot commented Feb 14, 2024

feat: rtl support for components [LIBS-525] #1448

feat: rtl support for components [LIBS-525] #1448

Conversation

tomzemp commented Feb 7, 2024 • edited Loading

Description

Known issues

Checklist

Screenshots

dhis2-bot commented Feb 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Feb 7, 2024 • edited Loading

Passing run #3210 ↗︎

Review all test suite changes for PR #1448 ↗︎

Choose a reason for hiding this comment

kabaros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhis2-bot commented Feb 8, 2024

dhis2-bot commented Feb 14, 2024

tomzemp commented Feb 7, 2024 •

edited

Loading

dhis2-bot commented Feb 7, 2024 •

edited

Loading

cypress bot commented Feb 7, 2024 •

edited

Loading