This repository has been archived by the owner on Sep 6, 2021. It is now read-only.
For Review. Starter feature. Locale descriptions in switching language modal. #3677
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
well i screwed up this pull request by pushing my changes to my master... (i fixed it for the fork repo). the commit (2fe98dd) is still there, so you could have a look at it. if its necessary i will open another pull request. sorry for the inconvenience. |
|
The request looks good. My only comment is that you should add the LOCALE names to the root strings and the LOCALE LABEL to the de one. Since when a languages doesn't have a string, it uses the root ones, and the translations are made based on the root ones. If possible, move the strings to be close to the other strings for the language modal dialog. If you want this to have a review and then a merge, you will need to create a new request. For just a review, this is not too bad. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I came here from the starter feature list and #1461 .
This is my first pull request ever, and its for review.
There is default behaviour to show just the locales (as it is now). It will lookup a i18n for a locale in the strings.js file of the language. I also added german translations for all current locales.
I think that the translation of every locale should be in that strings.js file as a part of the i18n (does not matter how it is displayed), even if the modal UI is refactored in some time.
I did not write on the mailing list before, because it is just a small hack.