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

Add support for RTL #96

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Add support for RTL #96

merged 2 commits into from
Nov 15, 2023

Conversation

evanwang0
Copy link
Contributor

@evanwang0 evanwang0 commented Sep 1, 2023 •
edited
Loading

This PR adds support for RTL and addresses issue #88. Feel free to request changes, overwrite, or ignore and close this PR. The PR is provided as a courtesy.

Here is a summary of the changes:

  • Adds rtl flag to init function
  • Adds enum WritingDirection
  • Adds properties right & bottom to FocusableComponentLayout
  • Refactors getChildClosestToOrigin to support search from the top right corner of the viewport based on writing direction
  • Refactors code depending on isIncrementalDirection to support RTL
  • Refactors code to use right/bottom
  • Adds support for rtl in the VisualDebugger

Notes:

  • I've removed the no-nested-ternary eslint rule as it's useful for concisely defining cases.
  • The main logical change in this PR is to getChildClosestToOrigin. All of the other changes should preserve logic in either writing direction. The changes to the other functions are primarily to preserve the meaning of the variable isIncrementalDirection. In other words, the booleans isVertical and isIncrementalDirection together map to left, up, right, down. However, with RTL support, isVertical, isIncrementalDirection, and writingDirection is needed to map to the same directions. These functions could be rewritten to use the direction directly so that logic is identical regardless of writing direction.

@evanwang0
Copy link
Contributor Author

Worth mentioning that the issue requests RTL on the fly and while not implemented in the PR can be done through replacing this.writingDirection with a form of document.dir

@predikament predikament added the enhancement New feature or request label Sep 21, 2023
@akash-kush9
Copy link

akash-kush9 commented Oct 11, 2023 •
edited
Loading

Worth mentioning that the issue requests RTL on the fly and while not implemented in the PR can be done through replacing this.writingDirection with a form of document.dir

Hi @evanwang0, Quick question regarding this PR, Once integrated will this allow the app to focus the rightmost element first instead of leftmost element?

@evanwang0
Copy link
Contributor Author

@akash-kush9 That is correct.

@jeev1989
Copy link

Thanks @evanwang0 👍 Tried out this fix and it seems to be working as expected.
@asgvard , @predikament : When can we expect this fix to be reviewed/merged and made available?

@jeev1989 jeev1989 mentioned this pull request Oct 26, 2023
Closed
@predikament
Copy link
Collaborator

Thanks @evanwang0 👍 Tried out this fix and it seems to be working as expected. @asgvard , @predikament : When can we expect this fix to be reviewed/merged and made available?

We've just added this task to our backlog and it will be reviewed ASAP; We'd like to get this functionality included in a good way.

@Braggiouy
Copy link
Contributor

Tested this PR locally, seems pretty complete and RTL layout quite well integrated.

Just as a hint for testing : https://www.aleksandrhovhannisyan.com/blog/changing-locale-in-chrome-with-dev-tools/

or

We can add dir="rtl" to the <html> tag once we inspect the app in the element inspector.

Quick Demo Video :

Grabacion.de.pantalla.2023-11-14.a.las.15.22.06.mov

@Braggiouy Braggiouy requested review from a team and Braggiouy November 14, 2023 15:23
@Braggiouy Braggiouy linked an issue Nov 14, 2023 that may be closed by this pull request
Support for RTL #88
Closed
predikament
predikament approved these changes Nov 14, 2023
View reviewed changes
Copy link
Collaborator

@predikament predikament left a comment

Choose a reason for hiding this comment

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

Looks fine to me as well.

@asgvard, @xavi160: Any opinions? ☺️

@sarah-harissa
Copy link

I am waiting for the approval, thank you guys.

@Braggiouy Braggiouy merged commit f087670 into NoriginMedia:main Nov 15, 2023
@predikament predikament mentioned this pull request Jan 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asgvard asgvard asgvard approved these changes

@predikament predikament predikament approved these changes

@Braggiouy Braggiouy Braggiouy approved these changes

Assignees

@evanwang0 evanwang0

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for RTL
7 participants
@evanwang0 @akash-kush9 @jeev1989 @predikament @Braggiouy @sarah-harissa @asgvard