Components: Fix RTL on Flex component

[walbo]

Description

The flex component has some margins to add gaps. These need to be reversed on RTL sites.

How has this been tested?

Tested different location that the component is used with Arabic and English.

Screenshots

Before	After

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[sarayourfriend]

Thank you for the fix!

[ciampo]

Thank you for working on this, @walbo !

For ease of review, noting that the only significant code changes are in the in the useFlex hook, where the marginLeft and marginRight CSS properties are being wrapped in the rtl() utility function (which is able to switch left and right CSS props when a RTL language is detected at component "mount time").

Every other code change is caused by auto-formatting (and consequently, by spacing updates in the unit test snapshots).

For completeness, it would probably be good to replace instances of the words "left" and "right" in the folder, so that the documentation also doesn't assume LTR languages. A few instances I found:

• gutenberg/packages/components/src/flex/types.ts

  Line 46 in 6a21c81

  * In the example below, `flex-start` will align the children content to the left.

• gutenberg/packages/components/src/flex/flex/README.md

  Line 68 in e6e0ea8

  In the example below, `flex-start` will align the children content to the left.

Finally, should we add a unit test for RTL styles? (maybe we could mock the isRTL function for that specific test, in order to force rtl styles to be applied?)

[walbo]

Looks like the left in the readme/types.ts refers to an example that doesn’t exists. I guess those can be removed.

I'll take a look at adding an RTL unit test later today (or tomorrow). @ciampo to you know if there is an existing test that has mocked RTL for reference?

[ciampo]

do you know if there is an existing test that has mocked RTL for reference?

Not that I know — but I'd probably look into mocking the isRtl function, maybe like this:

jest.mock( '@wordpress/i18n', () => ( {
	...jest.requireActual( '@wordpress/i18n' ),
	isRTL: jest.fn( () => true ),
} ) );

and then, maybe, later in the single test, change the implementation of isRtl to return true? Something like

isRTL.mockImplementationOnce( () => true );

... and either do a snapshot test of a component rendered with RTL locale, or maybe even do a diff snapshot between the LTR and the RTL version of the same snippet?

[sarayourfriend]

To be fair we don't do these kinds of tests literally anywhere else in Gutenberg, do we? I'm not sure they bring that much value to something as declarative as styles 🤷 The rtl utility itself should be tested and implementations of it should be able not to worry about that implementation detail (indeed I'd argue testing those styles would be testing an implementation detail).

The best way to test that RTL styles are working properly is probably something like visual regression testing with various locales. 🤔 But that's not something we have the infrastructure for at the moment.

[walbo]

I agree with @sarayourfriend about the test. Adding ex a snapshot test will only confirm that the rtl function works and rtl has is own test that should throw if anything doesn't work. I can add a test tomorrow if you still want it.

---

Added rtl.watch() but seems like rtl still needs to be executed.

packages/components/src/flex/flex/hook.js:88:14 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(template: TemplateStringsArray, ...args: CSSInterpolation[]): SerializedStyles', gave the following error.
    Argument of type '() => SerializedStyles' is not assignable to parameter of type 'CSSInterpolation'.
      Type '() => SerializedStyles' is missing the following properties from type 'ArrayCSSInterpolation': pop, push, concat, join, and 27 more.
  Overload 2 of 2, '(...args: CSSInterpolation[]): SerializedStyles', gave the following error.
    Argument of type 'TemplateStringsArray' is not assignable to parameter of type 'CSSInterpolation'.
      Type 'TemplateStringsArray' is not assignable to type 'CSSObject'.
        Types of property 'filter' are incompatible.
          Type '{ <S extends string>(predicate: (value: string, index: number, array: readonly string[]) => value is S, thisArg?: any): S[]; (predicate: (value: string, index: number, array: readonly string[]) => unknown, thisArg?: any): string[]; }' is not assignable to type '"inherit" | "none" | (string & {}) | "-moz-initial" | "initial" | "revert" | "unset" | ("inherit" | "none" | (string & {}) | "-moz-initial" | "initial" | "revert" | "unset" | undefined)[] | Filter[] | undefined'.
            Type '{ <S extends string>(predicate: (value: string, index: number, array: readonly string[]) => value is S, thisArg?: any): S[]; (predicate: (value: string, index: number, array: readonly string[]) => unknown, thisArg?: any): string[]; }' is missing the following properties from type 'Filter[]': pop, push, concat, join, and 27 more.

[ciampo]

To be fair we don't do these kinds of tests literally anywhere else in Gutenberg, do we?
...
I agree with @sarayourfriend about the test.

Sure, let's drop the unit tests.

Added rtl.watch() but seems like rtl still needs to be executed.

Is this Argument of type '() => SerializedStyles' is not assignable to parameter of type 'CSSInterpolation'. error happening even when the callback returned by rtl() is immediately executed (i.e. rtl({ ...styles })()) ? I can't see this error in the CI checks.

[walbo]

Is this Argument of type '() => SerializedStyles' is not assignable to parameter of type 'CSSInterpolation'. error happening even when the callback returned by rtl() is immediately executed (i.e. rtl({ ...styles })()) ? I can't see this error in the CI checks.

It work if I run rtl( { ...styles } )(), but without the final () (rtl( { ...styles } )) is fails. The PR still has the final () so not showing up here.

The example added in #33882 fails.

[ciampo]

It work if I run rtl( { ...styles } )(), but without the final () (rtl( { ...styles } )) is fails. The PR still has the final () so not showing up here.

The example added in #33882 fails.

This is expected. As mentioned in an earlier comment, interpolated functions only work for styled components.

Since in this case we're using the css function, we have to keep the final () for it to work properly. We're mitigating this "limitation" by adding the rtl.watch() to the list of useMemo dependencies, to ensure that styles are recalculated in case the locale's writing direction changes.

[walbo]

Thanks for the clarification @ciampo

I belive this is ready then.

[ciampo]

Code changes LGTM and test well as per instructions! Thank you @walbo for working on this 🚀

One last thing, before merging — could you add an entry for this bug fix in the "Unreleased" section of packages/components/CHANGELOG.md?