-
Notifications
You must be signed in to change notification settings - Fork 431
feat(ui): RTL support #7718
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.
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
base: main
Are you sure you want to change the base?
feat(ui): RTL support #7718
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 7e13bb9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| <html class="h-full"> | ||
| <html | ||
| class="h-full" | ||
| dir="rtl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For demo purposes, will revert before merging.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
📝 WalkthroughWalkthroughThe PR converts physical directional CSS properties to logical equivalents across the UI (e.g., left/right → insetInlineStart/insetInlineEnd; marginLeft/marginRight → marginInlineStart/marginInlineEnd; paddingLeft/paddingRight → paddingInlineStart/paddingInlineEnd; border radii → logical radii) and changes textAlign from left/right to start/end. It adds dir="rtl" to the sandbox HTML template, updates several icon/arrow transforms for RTL, and introduces an ESLint rule to enforce logical CSS properties. All changes are styling-related; component logic and public APIs remain unchanged. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/ui/src/elements/Avatar.tsx (2)
156-175:⚠️ Potential issue | 🟠 MajorReplace physical
leftwith logicalinsetInlineStartto keep RTL behavior correct.Anchoring the shimmer to the physical left breaks RTL alignment; it should start at the inline-start edge. Please switch to logical inset properties for both the wrapper and its
:afterto preserve RTL support.Proposed fix
position: 'absolute', top: 0, - left: 0, + insetInlineStart: 0, width: '25%', height: '100%', @@ position: 'absolute', top: 0, - left: 0, + insetInlineStart: 0, width: '400%', height: '100%',
25-37:⚠️ Potential issue | 🟠 MajorAdd/confirm RTL regression coverage for Avatar shimmer.
No tests were added/updated alongside this RTL-facing change. Please add or confirm existing visual/snapshot coverage for RTL positioning to prevent regressions. As per coding guidelines: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
🧹 Nitpick comments (1)
packages/ui/src/elements/TagInput.tsx (1)
112-176: Add minimal RTL regression coverage for these styling changes.No tests were added/updated in this PR; please add a basic RTL-focused snapshot/visual test to prevent layout regressions. As per coding guidelines: If there are no tests added or modified as part of the PR, please suggest that tests be added to cover the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/elements/Header.tsx (1)
1-113:⚠️ Potential issue | 🟠 MajorAdd RTL-focused tests before merge.
This PR changes RTL-facing UI behavior but adds no tests. Please add/adjust tests (e.g., RTL snapshot/visual regression for BackLink and arrow icons) to prevent regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ui/src/elements/Badge.tsx`:
- Around line 28-30: The badge X translate is not RTL-aware: in Badge.tsx update
the transform logic that currently uses translate(${t.space.$2x5}, ...) (paired
with insetInlineEnd) to handle RTL like Button.tsx does — wrap the transform in
a '[dir="rtl"] &' rule and provide the mirrored X value (or use scaleX(-1)) so
the horizontal offset is negated for RTL; keep the insetInlineEnd change and
ensure the selector targets the same class/element that currently has transform
applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ui/src/components/UserProfile/DeleteSection.tsx (1)
19-49:⚠️ Potential issue | 🟠 MajorAdd/confirm RTL coverage tests before merge.
This PR changes RTL styling but no tests are shown in the diff. Please add/adjust RTL visual or unit coverage (or point to existing tests) to validate these UI changes.
🤖 Fix all issues with AI agents
In `@packages/ui/src/components/UserProfile/PasswordSection.tsx`:
- Around line 39-43: Add an RTL-focused React Testing Library test for the
PasswordSection component to cover the paddingInlineStart behavior change:
render <PasswordSection /> (or the parent wrapper that sets passwordEnabled)
with dir="rtl" and with both passwordEnabled true and false, then assert the
computed style or DOM placement (e.g., check container.firstChild or the
specific element that receives the sx prop) for paddingInlineStart being '0'
when passwordEnabled is false and undefined/absent when true; use render from
`@testing-library/react` (and getComputedStyle or toHaveStyle assertions) and name
the test clearly (e.g., "renders correct paddingInlineStart in RTL") so this
regression is prevented.
Description
Preview: https://clerk-js-sandbox-git-alexcarpenter-rtl-support.clerkstage.dev/sign-in
Inspired by https://x.com/shadcn/status/2017287507881164816 update positioning values to use logical properties to better support RTL usage.
This also adds an eslint rule to prevent
noPhysicalCssPropertiesusage. Note some cases need to still use physical css properties and those have been disable with reasonings why.fixes #3961
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.