-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
RTL Support #1638
base: main
Are you sure you want to change the base?
RTL Support #1638
Conversation
Someone is attempting to deploy a commit to the shadcn-pro Team on Vercel. A member of the Team first needs to authorize it. |
This is interesting. I wonder if we could have this as an option via the cli. Turn on |
That's a great idea! The changes I have added result in support of both RTL and LTR directions, by simply adding Acheiving all of this via the cli would result in full RTL support, but I am not sure how can it be done. |
@nahasco I can take a look at this. Having this in the cli would be awesome. Adding it to the next milestone. I'll push to this PR and make sure you're credited for the work you've done here. Thank you. |
Regarding the choice between physical and logical properties, I'd like to clarify whether you intend to use logical properties as the default for all languages. Logical properties offer the flexibility needed for seamless direction switching, and I believe they could simplify the codebase and improve adaptability for various languages. For other specific elements like icons and HTML or radix direction values, there could be an option to manage these through the CLI to give a starting ground to the user. This approach allows us to make logical properties the primary choice for simplicity and flexibility while maintaining control over other elements that may require manual adjustments. I'm curious to know your thoughts on this approach and whether logical properties are indeed the direction you're leaning towards. Your input on this would be highly valuable. Once again, thank you for your contribution to this! |
please keep in mind the use case of supporting both LTR and RTL in the same codebase. |
It would be great to have it rn 😅 |
@@ -65,7 +65,7 @@ const AlertDialogFooter = ({ | |||
}: React.HTMLAttributes<HTMLDivElement>) => ( | |||
<div | |||
className={cn( | |||
"flex flex-col-reverse sm:flex-row sm:justify-end sm:space-x-2", | |||
"flex flex-col-reverse sm:flex-row sm:justify-end sm:space-x-2 rtl:space-x-reverse", |
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.
Why not flex flex-col-reverse sm:flex-row sm:justify-end sm:gap-2
? I think it'd be cleaner.
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.
I didnt want to change the original styling alot. Just adding rtl:space-x-reverse at the end is the most minimal, i think.
It's really shocking that a modern UI library doesn't fully support RTL or Internationalization (like calendar). Love shadcn but I with they would consider these stuff more seriously. |
@nahasco question. is there a 1:1 mapping for those utility classes? I wonder if we could write a transformer to do these on install? |
Thank you @shadcn
I am not sure what that means. But I didn't just adjust the classes, in some components I used radix ui's direction provider to pass the dir value to the component. For example the carousel component requires a I believe RTL support should be the default for the entire library and not just an option in the cli. The changes I made make the components support In a case where a user chooses not to support RTL in the cli, but later on decides to support RTL, they will have to refactor everything. So why not make RTL support the default? |
My app support both RTL and LTR, For my usecase, I updated the code for RTL support by globally replacing For the sidebar, I just changed the side based on the lang for dialogs, popovers, anything that would use a portal, i wrapped the children with a div with dir tag because it's not inherited from the root // src/components/ui/dialog.tsx
const DialogContent = React.forwardRef<
React.ElementRef<typeof DialogPrimitive.Content>,
React.ComponentPropsWithoutRef<typeof DialogPrimitive.Content>
>(({ className, children, ...props }, ref) => (
<DialogPortal>
<div dir={languageTag() == "ar" ? "rtl" : "ltr"}>
<DialogOverlay />
<DialogPrimitive.Content
ref={ref}
className={cn(
"fixed start-[50%] top-[50%] z-50 grid w-full max-w-lg rtl:translate-x-[50%] translate-x-[-50%] translate-y-[-50%] gap-4 border bg-background p-6 shadow-lg duration-200 data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] sm:rounded-lg",
className,
)}
{...props}
>
{children}
<DialogPrimitive.Close className="absolute end-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground">
<Cross2Icon className="h-4 w-4" />
<span className="sr-only">Close</span>
</DialogPrimitive.Close>
</DialogPrimitive.Content>
</div>
</DialogPortal>
)); Still facing small bugs here and there but nothing major |
Subscribing to this! |
Great work! |
Could you help with the remaining components? |
This is mazing following 🙏 |
This is similar to what I did in this PR. Additionally, I also added a dir state to some components and added |
@nahasco You are a legend! |
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.
Good job, i was thinking of use rlt[] and ltr[] but you solution required less code
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
RTL support for more components
Sidebar RTL support
RTL support for almost all components in the registry.
Mainly replaced physical properties with logical properties and added
rtl:space-x-reverse
wherespace-x-*
is used. In addition to using radix direction provider to return chevrons dynamically.This fixes #530, fixes #4877 , fixes #5776, fixes #5658 , fixes #2759 , fixes #2736 , fixes #1919
Up to date list of adjusted components:
"🔸" means the component didn't need any changes to be compatible with RTL layouts