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

Fix(rtl): better rtl support (#1236) #1381

Merged
merged 8 commits into from
Jul 27, 2024

Conversation

levyitay
Copy link
Contributor

Added RTL detection for text in view mode.

@julianpoy
Copy link
Owner

Hi there! Thank you very much for the PR!

Will give this a test. I'm not super familiar, but would the dir property (https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/dir) be a better fit here rather than text-align?

@julianpoy
Copy link
Owner

julianpoy commented Jun 5, 2024

So it looks like the same issue as reported in #1236 is still present with fraction symbols combined with numbers (3¾).

@levyitay
Copy link
Contributor Author

levyitay commented Jun 5, 2024

@julianpoy True, but I believe it has nothing to do with RTL, I might be wrong, but it might worth doing different PR for that.
Let me know if you want me to add it to this PR or open a different one

@julianpoy
Copy link
Owner

No worries! If you're willing to look into the issue with fractions I'd welcome it!

For the issue of supporting RTL text when language is set to LTR (as well as LTR text when language is set to RTL) - I don't believe text-align or the isRtlText method is necessary. The browser has built-in RTL/LTR detection, but does so by default on the whole document. If you set dir="auto" on any element within the document, the browser will re-check that element for RTL/LTR content.

In summary: If you swap out all of the [dir]="..." and [style.text-align]="..." properties for dir="auto", RTL/LTR cross-compatibility should work!

@julianpoy
Copy link
Owner

Wanted to follow up and say I appreciate the PR, and am happy to merge it as-is since it works great in my testing! If you'd like to swap out for the dir="auto" I'll leave that up to you - just let me know.

@levyitay
Copy link
Contributor Author

levyitay commented Jun 6, 2024

I'll give it a go in the weekend with dir="auto" I think it didn't work in my testing and report back

@levyitay
Copy link
Contributor Author

@julianpoy moved everything to dir="auto" sorry it took a while.
I left the method that detects if the text is RTL or not for later usage if needed.

@levyitay
Copy link
Contributor Author

@julianpoy should we merge it?

@julianpoy
Copy link
Owner

Hey there! Yep, can be merged. Thank you for the PR!

@julianpoy julianpoy merged commit c692b0c into julianpoy:master Jul 27, 2024
2 checks passed
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.

2 participants