Skip to content

Ensure Mac's titlebar padding always leaves space for the window controls - #603

Merged
p-jackson merged 3 commits into
trunkfrom
fix/rtl-titlebar-mac
Oct 16, 2024
Merged

Ensure Mac's titlebar padding always leaves space for the window controls#603
p-jackson merged 3 commits into
trunkfrom
fix/rtl-titlebar-mac

Conversation

@p-jackson

@p-jackson p-jackson commented Oct 16, 2024

Copy link
Copy Markdown
Member

Related to #541 and #596

This PR fixes the same issue as #596 except this time for Mac. Namely, the padding in the app's top bar needs to be aware of both the RTL mode of the language and the RTL mode of the underlying browser window.

Proposed Changes

  • Refactor isWindowFrameRtl logic into utility module
  • Create a <MacTitlebar> component that does the equivalent job of <WindowsTitlebar>
    • i.e. adjust padding to accommodate windows controls
    • Linux continues to the same topbar padding logic as Mac. No change here.
  • Ensure the padding we leave for Mac window controls is based on the browser window's language, not the language used by @wordpress/i18n etc.
  • Use a fixed pixel width for the window controls, rather than some number of rem
  • Did some fix ups in the site's sidebar to use logical properties, which can automatically deal with RTL switches.
    • I did a side-by-side comparison with trunk to confirm none of the padding sidebar padding has changed.

RTL language in a RTL window
ar-rtlCleanShot 2024-10-16 at 21 08 07@2x

LTR language in a RTL window
en-rtlCleanShot 2024-10-16 at 21 08 31@2x

LTR language in a LTR window
CleanShot 2024-10-16 at 21 19 34@2x

RTL language in a LTR window
ar-ltrCleanShot 2024-10-16 at 21 06 52@2x

Testing Instructions

Here's a quick way to change the browser window's language without having to reboot your OS into a different language: add app.commandLine.appendSwitch( 'lang', 'ar' ); at the very top of index.ts, right after the import statements.

  • Test a LTR language in a RTL window
  • Test a LTR language in a LTR window
  • Test a RTL language in a RTL window
  • Test a RTL language in a LTR window

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@p-jackson
p-jackson requested a review from a team October 16, 2024 08:20
@p-jackson p-jackson self-assigned this Oct 16, 2024

@sejas sejas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for making the changes for Mac. I tested it the four scenarios LTR and RTL language in the two "frames". Nice tip using app.commandLine.appendSwitch( 'lang', 'ar' );

Screenshots:

Screenshot 2024-10-16 at 10 31 07 Screenshot 2024-10-16 at 10 30 55 Screenshot 2024-10-16 at 10 30 01 Screenshot 2024-10-16 at 10 29 33
@p-jackson

Copy link
Copy Markdown
Member Author

app.commandLine.appendSwitch( 'lang', 'ar' );

Yeah it'd be nice to boot the app with the correct switch given the user's setting I think.

@p-jackson
p-jackson merged commit 2fc5879 into trunk Oct 16, 2024
@p-jackson
p-jackson deleted the fix/rtl-titlebar-mac branch October 16, 2024 10:26
@wojtekn

wojtekn commented Oct 16, 2024

Copy link
Copy Markdown
Contributor

Thanks for the fix @p-jackson !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants