Skip to content
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

Jump to bottom

feat: Add RTL support #110

Merged
merged 11 commits into from
Aug 25, 2023
Merged

feat: Add RTL support #110

merged 11 commits into from
Aug 25, 2023

Conversation

limitless-dev
Copy link
Contributor

@limitless-dev limitless-dev commented Jul 29, 2023 •
edited
Loading

This PR introduces RTL (Right-to-Left) support for the Toaster component.

direction

Related Issues

Changes:

  1. RTL adjustments for SVG icons within the Toaster.
  2. Corrected Acton button margins for RTL layout in Toaster.
  3. Support RTL through <html dir='rtl'>
  4. Support RTL through <Toaster dir='rtl' />
  5. dir attributes are rtl, ltr, auto
  6. Headless example changed to support rtl (through Toaster or html)

Priority of direction (dir) :

  1. Toaster
  2. html
  3. if both not set, default to ltr
<Toaster dir="ltr" />
<Toaster dir="auto" /> // html direction or ltr
<Toaster dir="rtl" />
<Toaster /> // html direction or ltr
<html dir="rtl">
<html dir="ltr">

I know we can hack this with CSS, but it's so much nicer to have it built-in. Saves us from redoing it in every project.

@vercel
Copy link

vercel bot commented Jul 29, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sonner ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 25, 2023 5:00pm

joaom00
joaom00 reviewed Jul 30, 2023
View reviewed changes
Copy link
Contributor

@joaom00 joaom00 left a comment

Choose a reason for hiding this comment

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

Hey @limitless-dev, thanks for your contribution. Just some suggestions

  • I think we can refactor to use logical properties here.

  • Some alignments are incorrect (I think this should be fixed using logical properties):
    image

  • Add dir type to Toaster props

@limitless-dev
Copy link
Contributor Author

Hi @joaom00 - Thanks for taking the time to review!

I think we can refactor to use logical properties
Some alignments are incorrect

I just checked again and it appears to work on Firefox but not Chrome!
I'll see what I can do.

Add dir type to Toaster props

Actually, that was my first approach but I realized I'll need to pass dir='rtl' to both:

  1. Toaster: since I assume will be defaulting the direction prop as LTR.
  2. html: since that's usually the default way of enabling RTL

Beside, that can be done through the style prop:

<Toaster
  style={{ direction: 'rtl' }}
/>

So I focused on the styling issues for SVG and action button.

@limitless-dev
Copy link
Contributor Author

Fixed the issue, RTL should now function correctly (limited to HTML direction only).
Please let me know if having the dir as prop for Toaster is a preferable approach. Thank you.

joaom00
joaom00 reviewed Aug 1, 2023
View reviewed changes
Copy link
Contributor

@joaom00 joaom00 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please let me know if having the dir as prop for Toaster is a preferable approach

I think we can keep on html only. (Maybe we can add a dir prop to the Toaster and in the website add an option to enable the RTL mode)

One thing missing is inverting [data-close-button] as well and, if you don't mind, can you invert the position of the dismiss button in the Headless example? Thanks

Something I'm in doubt is whether we should invert the position of the toast too 🤔

@limitless-dev
Copy link
Contributor Author

limitless-dev commented Aug 1, 2023 •
edited
Loading

Hi @joaom00 , thanks for your comments.
I have added the dir as prop to Toaster and revamped the styling for readability.

Priority of dir assignment:

  1. Toaster
  2. html
  3. if both not set, default to ltr

Changes

  • dir support for Toaster (ltr and rtl and auto)
  • styling icon and button with CSS variables based on directions
  • Headless example now supports rtl (through Toaster or html)

and in the website add an option to enable the RTL mode

Once we finalize this as stable, I'll add an example 👍

emilkowalski
emilkowalski reviewed Aug 25, 2023
View reviewed changes
website/src/components/Direction/index.tsx Outdated

export type Direction = (typeof directions)[number];

export const Direction = ({
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove this and add it to the Readme instead

@emilkowalski
Copy link
Owner

This is awesome, left one comment. Can we add tests for this as well?

@limitless-dev
Copy link
Contributor Author

This is awesome, left one comment. Can we add tests for this as well?

Hi @emilkowalski, thank you for your review.

  • Removed Direction example from website
  • Added Direction guide to README
  • Added 3 tests for dir prop
    • toaster's dir prop is reflected correctly
    • toaster respects the HTML's dir attribute
    • toaster respects its own dir attribute over HTML's

@emilkowalski
Copy link
Owner

Amazing, thank you so much for this

@emilkowalski emilkowalski merged commit d803cf5 into emilkowalski:main Aug 25, 2023
2 checks passed
@yasseralsaidi
Copy link

yasseralsaidi commented Dec 13, 2023 •
edited
Loading

please see this issue related
#254

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

@joaom00 joaom00 joaom00 left review comments

@emilkowalski emilkowalski emilkowalski left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@limitless-dev @emilkowalski @yasseralsaidi @joaom00