terriajs icon indicating copy to clipboard operation
terriajs copied to clipboard

Add Arabic translation with RTL support

Open MoRadwan74 opened this issue 2 years ago • 15 comments

What this PR does

Enable Arabic translation for user interface while handling RTL styles.

Test me

You can check this PR by checkout this branch and setting up a clean TerriaMap following the normal guide available in the docs while setting the Arabic language in the languages configuration in config.json as below.

"languageConfiguration": {
  "enabled": true,
  "debug": false,
  "languages": {
    "en": "English",
    "fr": "french",
    "ar": "العربية"
  },
  "fallbackLanguage": "en"
},

Checklist

  • [x] There are no unit tests to verify my changes are correct or unit tests aren't applicable.
  • [ ] I've updated relevant documentation in doc/.
  • [x] I've updated CHANGES.md with what I changed.
  • [x] I've provided instructions in the PR description on how to test this PR.

MoRadwan74 avatar Mar 28 '22 13:03 MoRadwan74

@steve9164 Thanks a lot for reviewing this PR.

We will update our PR according to your notes and get back to you as soon as possible.

MoRadwan74 avatar Mar 31 '22 06:03 MoRadwan74

Dear @steve9164, @abdallahossam1998 made the changes per your notes. Please, check.

MoRadwan74 avatar Apr 03 '22 11:04 MoRadwan74

Hi @MoRadwan74, @steve9164 Just to check, this will require all developers to implement RTL styling, and add a review point RTL styling. Have we considered using something like https://www.npmjs.com/package/stylis-plugin-rtl, it is maintained by styled-components.

zoran995 avatar Apr 03 '22 13:04 zoran995

Hi @zoran995, I checked it and found it very interesting! but we need to make sure it successfully converts the styles to RTL.

MoRadwan74 avatar Apr 04 '22 21:04 MoRadwan74

Hello @steve9164, any updates?

MoRadwan74 avatar Apr 10 '22 08:04 MoRadwan74

Using a plugin to handle RTL styling sounds like a better approach than manually adding RTL styles across many UI components. I think https://www.npmjs.com/package/stylis-plugin-rtl looks promising, but I don't have much experience with RTL languages or styling. Can anyone try out that plugin?

steve9164 avatar Apr 11 '22 16:04 steve9164

Hello @steve9164, I will check this package and get back to you.

abdallahossam1998 avatar Apr 12 '22 09:04 abdallahossam1998

Hello @zoran995 @steve9164,

I've tried using https://www.npmjs.com/package/stylis-plugin-rtl package and I've some notes about it

1- The package only worked in v1.0 for styled-components v5, I tried to upgrade it to v2.1.1, but the package has no effect.

2- It throws an error in the .tsx components When I use "rtlPlugin from stylis-plugin-rtl", I get type error:

   Type '((context: number, content: string) => string | undefined)[] | undefined' is not assignable to type 'StylisPlugin[] | undefined',

when I upgraded it to v2.1.1 the error is gone but the package won't work.

3- It works with styled components, it changes basic CSS properties i.e. "float: left" to "float: right" etc, but most of the styles that depend on variables i.e. "TourPortal" component doesn't work.

4- Most of the commits I've added RTL styles to SASS styles, so I tried to apply the package in components with SASS styles not working in most of them but worked in "HelpPanel" component because it has inline styled component CSS and not working very well only applied to inline styled component CSS.

5- It worked in 8 components but there is type error thrown in .tsk components.

abdallahossam1998 avatar Apr 20 '22 12:04 abdallahossam1998

Hi, @steve9164 Any updates regarding this PR?

MoRadwan74 avatar May 08 '22 08:05 MoRadwan74

Hi @MoRadwan74 and @abdallahossam1998. Thanks for the contribution. We will merge this PR to add RTL support, however our team can't commit to in-house RTL support for future features (we'd welcome further contributions to improve RTL support). We'll try to review and merge in early June (we have some important demos coming up and are avoiding large changes before June 1).

steve9164 avatar May 19 '22 00:05 steve9164

Hello @steve9164, thanks a lot for your effort as a team. Please inform us if you want any changes to this PR and sure we are happy and ready to contribute regarding RTL styles.

Good luck with your demos!

MoRadwan74 avatar May 26 '22 15:05 MoRadwan74

Hi @MoRadwan74. I've merged latest main into your branch and pushed it to cl-arabic-rtl. It's deployed to http://ci.terria.io/cl-arabic-rtl/ for testing.

I also read up RTL on the web. These articles were interesting to me: https://dev.to/pffigueiredo/series/13921.

I think we should use the https://www.npmjs.com/package/stylis-plugin-rtl package, thanks for testing. We're slowly phasing out SASS in favour of styled-components, so the plugin will become more and more useful (hopefully). I got around the TypeScript errors using:

<StyleSheetManager
  stylisPlugins={
    i18n.dir() === "rtl"
      ? [(stylisRTLPlugin as unknown) as StylisPlugin]
      : []
  }
>

I don't know at what level in the UI this plugin should be applied. Should it wrap each major UI component? Or a larger tree of components?

For layouts that are calculated using variables I think RTL logic will have to be introduced (as you've done for TourPortal).

I've had a look at browser support for margin-inline-start and other CSS Logical Properties and I'm undecided whether to allow using those (old Safari & Chrome versions still have some usage, and work with all other elements of TerriaJS).

Next steps:

  • [ ] Propose and agree on an approach for RTL styles that we can put into our developer documentation
  • [ ] Make a decision on using CSS Logical Properties (@steve9164)

steve9164 avatar Jul 21 '22 10:07 steve9164

Hi @MoRadwan74. We have left comments for you in the PR. Please respond to ensure progress on the PR. thank you.

AnaBelgun avatar Aug 31 '22 03:08 AnaBelgun

Hello @steve9164

  • I've tried to remove !important but it didn't work for RTL styles and for related maps panel component if you removed showDropdownInCenter the problem is gone.
  • I've tried the stylis-plugin-rtl package it worked with basic styles like mapNavigation component it applies the RTL but all the buttons are LTR, so it's not functionally working for me.

abdallahossam1998 avatar Sep 01 '22 00:09 abdallahossam1998

Hi @abdallahossam1998 and @MoRadwan74 how are things progressing with this ticket? There are three points on which we would like some thoughts from you:

I think we will need to find an alternative to using the !important rule if we are going to merge this into the Terria master branch for maintainability reasons... Is there some alternative you can find?

Regarding stylis-plugin-rtl could you please elaborate on "I've tried the stylis-plugin-rtl package it worked with basic styles like mapNavigation component it applies the RTL but all the buttons are LTR, so it's not functionally working for me". Hopefully there is a way to get it to work for the buttons too, and we can move forward with this approach.

Could you please reply with your thoughts on this comment from @steve9164 : https://github.com/TerriaJS/terriajs/pull/6216#issuecomment-1191308797

Thanks for all your hard work so far!

staffordsmith83 avatar Sep 29 '22 00:09 staffordsmith83