Skip to content

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

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.

Sign up for GitHub

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
ciampo merged 6 commits into from
May 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bd68408
`BorderBoxControl`: fix right border alignment on tablet/mobile viewp…
ciampo May 23, 2022
1b04d6b
Fix useMemo missing dependencies
ciampo May 23, 2022
8930b9f
Make RTL styles dynamic functions
ciampo May 23, 2022
69ae1a8
CHANGELOG
ciampo May 23, 2022
597f959
Add eslint ignore regarding the exhaustive deps rule
ciampo May 23, 2022
1bf6c29
Merge branch 'trunk' into fix/border-box-control-right-alignment
ciampo May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add eslint ignore regarding the exhaustive deps rule
  • Loading branch information
@ciampo
ciampo committed May 23, 2022
commit 597f95905b7311ca660d16f484916a36b10f5be4
9 changes: 7 additions & 2 deletions packages/components/src/border-box-control/border-box-control-split-controls/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,22 @@ export function useBorderBoxControlSplitControls(

// Generate class names.
const cx = useCx();
const rtlWatchResult = rtl.watch();
const classes = useMemo( () => {
return cx( styles.borderBoxControlSplitControls(), className );
}, [ cx, className, rtl.watch() ] );
// rtlWatchResult is needed to refresh styles when the writing direction changes
// eslint-disable-next-line react-hooks/exhaustive-deps
Comment on lines +28 to +29
Copy link
Contributor Author

@ciampo ciampo May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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 )

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw May 24, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ciampo ciampo May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@chad1008 chad1008 May 24, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ciampo ciampo May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@chad1008 chad1008 May 24, 2022

Choose a reason for hiding this comment

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

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 😄

Copy link
Member

@mirka mirka May 27, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ciampo ciampo May 27, 2022

Choose a reason for hiding this comment

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

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

}, [ cx, className, rtlWatchResult ] );

const centeredClassName = useMemo( () => {
return cx( styles.CenteredBorderControl, className );
}, [ cx, className ] );

const rightAlignedClassName = useMemo( () => {
return cx( styles.rightBorderControl(), className );
}, [ cx, className, rtl.watch() ] );
// rtlWatchResult is needed to refresh styles when the writing direction changes
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ cx, className, rtlWatchResult ] );

return {
...otherProps,
Expand Down
5 changes: 4 additions & 1 deletion packages/components/src/border-box-control/border-box-control-visualizer/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,15 @@ export function useBorderBoxControlVisualizer(

// Generate class names.
const cx = useCx();
const rtlWatchResult = rtl.watch();
const classes = useMemo( () => {
return cx(
styles.borderBoxControlVisualizer( value, __next36pxDefaultSize ),
className
);
}, [ cx, className, value, __next36pxDefaultSize, rtl.watch() ] );
// rtlWatchResult is needed to refresh styles when the writing direction changes
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ cx, className, value, __next36pxDefaultSize, rtlWatchResult ] );

return { ...otherProps, className: classes, value };
}