Add support for RTL - Githubissues

evanwang0 commented 1 year ago

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 commented 1 year ago

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

akash-kush9 commented 12 months ago

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 commented 11 months ago

@akash-kush9 That is correct.

jeev1989 commented 11 months ago

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?

predikament commented 11 months ago

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 commented 10 months ago

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 :

https://github.com/NoriginMedia/Norigin-Spatial-Navigation/assets/72386708/ea957df4-5070-4f54-9e62-28e468be38e4

sarah-harissa commented 10 months ago

I am waiting for the approval, thank you guys.

NoriginMedia / Norigin-Spatial-Navigation

Add support for RTL #96