BorderBoxControl: fix right border control alignment on small viewports and RTL styles

[ciampo]

What?

This PR:

• Fixed the alignment of the right border control in the BorderBoxControl component on small viewports (This fix was introduced by @aaronrobertshaw as part of https://github.com/WordPress/gutenberg/pull/40874/files/5a621366b48221394fe608ce3872d71ce80fd19d#r866514918, but the original PR didn't get merged.)
• Fixes the application of RTL styles to BorderBoxControl by adding missing useMemo dependencies and by converting serialised Emotion style objects to dynamic functions

The bug fixes in this PR also related to #41166

Why?

Bug fix

How?

The styles used to align the right border are the same used by @aaronrobertshaw in the original PR (#40874).

While adding and testing these changes, I noticed that RTL styles where not applied as expected, and found out a few issues with the way styles are written and assigned.

Testing Instructions

• Render the BorderBoxControl component (either in Storybook, or in the editor)
• Unlink the sides, so that 4 border controls are rendered (top, right, bottom, left border)
• On a small screen (mobile/tablet), make sure that the right border control is aligned correctly with the rest of the UI
• Using the tools of your choosing, test that styles are applied correctly also when the writing direction is RTL

Screenshots or screencast

Control alignment:

trunk	This PR

(the change is subtle, but in this PR, the right border control is a bit further away from the left border control)

RTL styles

border-box-control-rtl.mp4

(first Storybook is trunk, second Storybook is this PR)

[ciampo]

Added a bunch of these eslint-disable-next-line rules in preparation for the exhaustive-deps rule (see #41166)

@sarayourfriend , WDYT about the current rtl.watch() approach? Asking because, with the new eslint rule, we may need to have to "ignore" the rule a lot of the times, which defeats the point a little bit

(cc @chad1008 )

[aaronrobertshaw]

It does defeat the purpose of adding the exhaustive-deps rule if all components should eventually support RTL. In the interim though probably better to catch a few oversights in the dependency arrays.

[ciampo]

I agree. Ignoring the rule for now is a lesser evil compared to not adding the eslint rule at all. Will keep it as is for now

[chad1008]

Want to confirm I'm following the logic here :)

rtlWatchResult isn't actually called in the useMemo's create function, so it's not technically a dependency from React's POV.

We do, however, want to recalculate classes whenever rtlWatchResult changes, so we add it to the dependency array to trigger the useMemo, which exhaustive-deps will hate for the reason described above?

[ciampo]

Correct.

A couple of alternative approaches that come to mind:

• we could make these "dynamic style" functions accept, as an argument, a isRTL boolean (for example: return cx( styles.borderBoxControlSplitControls( rtlWatchResult ), className );). This would make the computation of RTL styles less "auto-magical", which isn't necessarily a bad thing since we've been missing a few of those!
• create a hook that forces a re-render of the component when the writing direction changes, and therefore remove it from the list of dependencies of useMemo

Thoughts?

[chad1008]

The first option sounds, to me, like it might make things a little more intuitive, but I'd happily defer to others who've worked more with the current auto-magical approach 😄

[mirka]

I feel like we've discussed this before so sorry if I'm forgetting a crucial piece of the convo, but do we actually have a good reason to memoize these things at all? I searched around but couldn't really find anything that mentioned any performance issues that were observed. The Emotion folks also don't recommend blanket optimizations like this.

[ciampo]

That is also a good question. From what I remember, the reason why we memoize Emotion classname computation is because of a test ran by Q before I joined the components squad, where he found out that computing these classnames can be quite expensive.

Since then, quite some time has passed and, with that, libraries (like Emotion and React) also got updated. Therefore, I'm not sure if this assumption still holds true. We should run some tests and take it from there

[aaronrobertshaw]

Thank you very much for addressing this @ciampo, it had slipped off my radar 🙇

✅ BorderBoxControl functions and looks correctly aligned in the Storybook
✅ Border controls in the editor look and work well in the editor with RTL or small viewports
✅ Unit tests are still passing
✅ No linting errors (other than those addressed by #41259)

LGTM 👍

[aaronrobertshaw]

It does defeat the purpose of adding the exhaustive-deps rule if all components should eventually support RTL. In the interim though probably better to catch a few oversights in the dependency arrays.