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

[locales/fr.json] correct some typos on french strings #953

Merged
merged 8 commits into from
Aug 1, 2022

Conversation

Badatos
Copy link
Contributor

@Badatos Badatos commented Jul 19, 2022

  • [Accessibility] add missing labels on some "select" boxes
  • string "Language" (in settings) can now be localised

Corrections on french strings :

  • add missing non breakable spaces before double ponctuations
  • correct some accords (like "la vidéo téléchargé" -> "la vidéo téléchargée")
  • use curvy quote (see http://cesarx.free.fr/de_la_typo.html)

* add missing non breakable spaces before double ponctuations
* correct some accords (like "la vidéo téléchargé" -> "la vidéo téléchargée")
* use curvy quote (see http://cesarx.free.fr/de_la_typo.html)
+ string "Language" can now be localised
@Badatos
Copy link
Contributor Author

Badatos commented Jul 20, 2022

Hello There. This PR has been tested and is ready for review. Have a nice day ;)

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Hi there and thanks for your PR! As with all languages that I don't know, I obviously can't verify these changes, but it doesn't look like you sneaked any swearwords into this 😁

I only have one comment below. After that is resolved, I'm happy to merge.

Comment on lines 14 to 22
<SettingsSection title="Language">
<SettingsSection title={i18n.t('settings-general-header')}>
<label
htmlFor='studio-lang'
sx={{ variant: 'styles.label' }}
>
{i18n.t('settings-language-label')}
</label>
<select
id="studio-lang"
Copy link
Member

Choose a reason for hiding this comment

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

We actually explicitly kept "Language" untranslated. If you set Studio to some weird language, you still want to find the language selection, and we figured most people know what "language" means. We plan on improving this a lot in the future via some redesign.

So, could you remove this part of the change (including the new translation strings).

Regarding adding a label to this and other drop-down menus: is this mainly for accessibility? I'm not quite sure what this change is for. I'm fully on board with the change for the audio and video device (as it doesn't change anything about what's visible), but I'm not sure it's useful in this language case?

Copy link
Contributor Author

@Badatos Badatos Aug 1, 2022

Choose a reason for hiding this comment

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

Ok ! It make sense, yes ;) Maybe we can keep both : translated string + "Language" between parenthesis ?

Adding the labels is mandatory for accessibility yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to evaluate accessibility level of a web page, I recommend the "Wave" tool : https://wave.webaim.org/ which can be installed as a plugin in your web browser.

Copy link
Member

Choose a reason for hiding this comment

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

I do know Wave and have it permanently installed actually. I guess I just never checked these Studio things, probably because due to the lack of time, but likely also because not long ago, Studio had way more serious accessibility concerns :D

…e weird language, you still want to find the language selection, and we figured most people know what "language" means. We plan on improving this a lot in the future via some redesign.)
@Badatos
Copy link
Contributor Author

Badatos commented Aug 1, 2022

I just made a commit which display "Language" string both translated + untranslated. Is it ok for you ?

@LukasKalbertodt
Copy link
Member

Yep, keeping both sounds good! But can you hide the second / Language when the language is English? Should be easy to do and looks really weird otherwise. And while you're at it, could you replace the first line with const { i18n, t } = useTranslation(); (note the added t) and then use t instead of i18n.t for translating? It's a minor thing, but makes it consistent with the existing code. Then I'm happy to merge!

@Badatos
Copy link
Contributor Author

Badatos commented Aug 1, 2022

Yep, keeping both sounds good! But can you hide the second / Language when the language is English? Should be easy to do and looks really weird otherwise. And while you're at it, could you replace the first line with const { i18n, t } = useTranslation(); (note the added t) and then use t instead of i18n.t for translating? It's a minor thing, but makes it consistent with the existing code. Then I'm happy to merge!

You're right : it looks weird in english ;) I try to hide it on this case.

+ use t instead of i18n.t for translating (makes it consistent with the existing code.)
@Badatos
Copy link
Contributor Author

Badatos commented Aug 1, 2022

Here's the new commit including :

  • Hide the second "/ Language" when the language is English
  • use t instead of i18n.t for translating (makes it consistent with the existing code.)

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Terribly sorry to request changes again :D But for languages where "language" is not translated, it falls back to English, so they still have the "Language / Language" problem. As shown below, I would just literally check whether the string is "English".

src/ui/settings/language.js Outdated Show resolved Hide resolved
@LukasKalbertodt
Copy link
Member

I was so free as to just apply my suggestion. Now I'm happy :D Thank you again for the PR!

@LukasKalbertodt LukasKalbertodt merged commit d0b81ea into elan-ev:master Aug 1, 2022
@Badatos
Copy link
Contributor Author

Badatos commented Aug 1, 2022

Thank you for the merge and your review : all your remarks are pertinent and it's how we make things better ;) Have a nice day !

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