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.

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

Add support for RTL #907

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add support for RTL #907

wants to merge 4 commits into from

Conversation

bakura10
Copy link

@bakura10 bakura10 commented Oct 10, 2023

Hi !

This is an attempt to add support for RTL. I have not tested all possibilities but it seems to work well on the few cases I tried. While I ported everything to logical properties, I only tried the RTL (I am not aware of any language reversing the block order).

Here are the main changes that have been done:

  • Using logical property instead of non-logical properties when applicable (eg. scrollMarginLeft => scrollMarginBlockStart)
  • Normalize properties that do not have logical equivalent:
    • getBoundingClientRect are reversed (left becomes right and vice versa) and normalize with the viewport width
    • scrollLeft is normalized as it returns negative values in RTL context
  • Finally, at the final step, the left scroll is negated as in RTL scrolling requires negative value

I decided to keep "left" and "top" at the final step (what is returned to calculations) because the native scrollIntoView, scrollTo, scrollBy... do not support logical property so it would be confusing for the library consumer.

I am not 100% confident of all the changes, and I have no guarantee that all situations work properly, but this should be a good start to provide RTL feature.

EDIT: for the implementation I decided to rely on "document.dir" instead of having an extra option in the library. I know some librairies explicitly require a direction option, but a lot of thing break in RTL context if the document does not have the dir="rtl" attribute set, so I think we should do this to encourage people to add the correct attribute on the document.

@bakura10
Copy link
Author

Hi @stipsan , any info on whether this could be merged or not? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant