Closed keichan34 closed 3 months ago
The main pain point right now is the warning in the console for the auto complete component. If you can fix that before/during introduction of i18n it would be great. Also, if there's a component library that can be used that supports i18n out of the box it might be a direction worth exploring.
@HarelM Thanks. I'll check out the auto complete component while I'm working on this, but doing a cursory search, it looks like there might not be much to translate in the auto complete component? I think it's just being used for values in the style itself -- not for field labels, etc? Unless I'm misunderstanding something?
It's less about the i18n of the auto complete component, it more about replacing it with something that doesn't create console warning. I get why it might not be relevant at all for this PR though. I'm not a react expert, so I was hoping someone like you could easily fix this.
Thanks for taking the time to open this PR! Can you add a screenshot of the language button? Can you also add a e2e test to check language switch and make sure the text is changed?
Also I'm not sure what should happen for RTL languages - i.e. translating to Hebrew and Arabic requires more than simple string substitution, and that's why I asked about using a component library that supports i18n, or something similar. IDK...
I haven't had much experience doing translation in RTL languages, if you know anyone who uses RTL and can help us out, that would be great. I think really supporting RTL would be a bit more involved than just replacing strings, like you said -- we'd probably have to change the layout of the map to be on the left, the toolbar, etc, so there would probably be some CSS and manual i18next.dir() === 'rtl' ? ... : ...
usage here and there...
Will be adding E2E tests. Here's what the language button/menu looks like now.
Let me know if maybe it should go somewhere else. I just put it here because it was easy to copy-and-paste the view menu.
Thanks for taking the time to take a look at the PR.
I'm speaking Hebrew so I can help out in terms of RTL.
It does require more work by adding the dir
attribute to all kind of places in the code and have this configured globally somehow.
I have an app which is uses angular material and is fully RTL and LTR compatible here: https://israelhiking.osm.org.il/ but the extra dir
is not an easy fix.
It would be nice however to include the ability to add dir=...
as part of this PR.
@HarelM Took a look at isrealhiking. I see. I did some initial RTL support, can you take a look at it and see if the general direction is OK? It looks like if flexbox is used for layout, most LTR->RTL transformations are "free" by just setting document.body.dir = "rtl"
. The main layout was using position: absolute, so I refactored it to use display: flex, and it looks like it's working, although there may be some minor problems. I'll be working on them in the coming days.
I'm not familiar with RTL, so let me know if there should be any more major layout fixes. Maybe the carets to expand sections and the labels of the sections / layer names should be reversed as well?
I've added Hebrew translation, please remove the survey translations and the relevant component as it is no longer in use.
Attention: Patch coverage is 58.82353%
with 49 lines
in your changes missing coverage. Please review.
Project coverage is 58.31%. Comparing base (
f34529e
) to head (13d0b04
). Report is 8 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for taking care of the Firefox annoying test failures. If they continue to fail I'll simply remove them from CI. Do you want this merged or are you planning more changes?
Can you also delete the following file and remove references to it?
ModalSurvey.tsx
Thanks!
Let me know if there is anything else I should brush up.
Let's push it and see how it behaves :-)
Looks nice! A lot better than I expected!
I think "zoom:" is missing in the zoom component. Search placeholder for the geocoder control too (I'm now updating it with typescript, so it should be easier soon)
We might want to add a test to see that all the keys exist in the translation file, it should be a relatively simple test.
P.S. the word Language should be kept in english, otherwise if you accidentally change the language, it's hard to understand where to click in order to revert to English. "שפה" in the above screenshot.
Ah: I forgot. The maplibre controls only pick up the translation on initialization, so if you refresh the page, it should work. It has to do with the controls being outside of React's area of control, so I'll have to listen for the i18next events to make switching working with them. I think we should make a tracking issue? or individual issues? for fixes like these? What do you think?
Sure, tracking issues should be better. If the map controls are hard to update I wouldn't stress about them, your call.
This is a rough start on adding react-i18next. I'll be working on adding more translatable strings and translations in the coming days. I'm going to need to wrap class components in HOCs, so let me know if there's something I should be fixing before doing that. I'm thinking now to keep the exported class names exactly the same, and rename the existing classes by prefixing an
I
(for internal). For example:becomes
I'll be able to contribute Japanese strings (I've talked to a couple people on my team and they'll be happy to help as well), so that's the language I decided to go with in this PR.
Closes #746