-
Notifications
You must be signed in to change notification settings - Fork 550
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
Add localization #2748
base: main
Are you sure you want to change the base?
Add localization #2748
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on where you are with the new localization packages I think its reasonable to land something like this in DartPad.
We probably also want some text in the README.md, "To regenerate the localization files, run dart run build_runner build --delete-conflicting-outputs
.".
|
||
flutter: | ||
uses-material-design: true | ||
assets: | ||
- assets/dart_logo_128.png | ||
- assets/flutter_logo_192.png | ||
- lib/l10n/en.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but we should add some docs to the repo about how to contribute a new localization; this will be a good way to get more people involved.
@@ -137,6 +138,14 @@ class _DartPadAppState extends State<DartPadApp> { | |||
routerConfig: router, | |||
debugShowCheckedModeBanner: false, | |||
themeMode: themeMode, | |||
localizationsDelegates: [...MessagesLocalizations.localizationsDelegates], | |||
supportedLocales: const [ | |||
Locale('en'), // English |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for this PR, but it may be nice to have some CI checks that this list matches the arbs files available, and the list of json files in the flutter assets section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how Flutter currently handles this matching between supportedLocales
and actually existing .arb
files. @HansMuller?
I don't know how developers with RTL languages as the set locale code - @Manishearth? |
It shouldn't overlap the text, it's a code box and the code is LTR. If you want to support RTL programming languages it gets a bit more theoretical. Dart has a default LTR direction and should stay that way even when RTL text is used in it, since its keywords are LTR. |
Thanks! I think it's just about RTL languages. So then always aligning these buttons to the right seems like the correct choice. |
This PR should address the issue w/ the buttons in a rtl locale: #2754. |
@mosuem This is really exciting, thanks for working on this (and the underlying tooling). I'm curious if you'd be comfortable with some form of this landing soonish? It doesn't seem super invasive since there's not a lot of strings in the app, and I think we would be fine with making changes as needed, at least for now. That way we can: be better prepared for any upstream/longer-term solution, engage the community with localizations, and have a simple test-bed for at least some portions of your work. |
Sure! This would first need a button to switch locale at some place - any suggestions where? |
Perhaps to the right of the channel selector widget? We do have about as much UI clutter as we want in DartPad's UI - part of its strength is its simple UI. We could also consider moving the theme toggle and the locale selector to the overflow menu. But we could start with the locale selector on the bottom toolbar and see how well that works. |
I think a little globe icon without text (but still a semantic label) to the right of the channel selector makes a lot of sense! If it doesn't work out, we can definitely move it to the overflow menu. I imagine it will be relatively rare to switch. Side question: Even if we don't add any client analytics, does the hosting setup show the traffic to individual files? If so, we could use the downloads of the different json files as a useful reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said, I'm super excited for this. Thanks so much!
I'm all for this landing if @devoncarew is happy, but I have a few questions more about the underlying tooling, nothing blocking this.
Could you make sure each string is configurable and setup for at least German too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no open questions w/ this PR? lgtm; excited about the change to get code completion on messages ('Reload'
==> messages.reload
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts from a stranger.
* Set the `"locale"` key in the file to `ar`. | ||
* Add the translated strings to the file. Use the descriptions in `en.arb` as a reference. | ||
* Add the locale to the `supportedLocales` in `main.dart`. | ||
* Add `lib/ar.json` to the assets in the `pubspec.yaml` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be
* Add `lib/ar.json` to the assets in the `pubspec.yaml` | |
* Add `lib/l10n/ar.json` to the assets in the `pubspec.yaml` |
if (result.source == value) { | ||
appModel.editorStatus.showToast('No formatting changes'); | ||
appModel.editorStatus | ||
// ignore: use_build_context_synchronously | ||
.showToast(context.messagesLocalizations!.noFormattingChanges); | ||
} else { | ||
appModel.editorStatus.showToast('Format successful'); | ||
appModel.editorStatus | ||
// ignore: use_build_context_synchronously | ||
.showToast(context.messagesLocalizations!.formatSuccesful); | ||
appModel.sourceCodeController.text = result.source; | ||
} | ||
} catch (error) { | ||
appModel.editorStatus.showToast('Error formatting code'); | ||
appModel.editorStatus | ||
// ignore: use_build_context_synchronously | ||
.showToast(context.messagesLocalizations!.errorFormatting); | ||
appModel.appendLineToConsole('Formatting issue: $error'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to ignore this lint, use the mounted check instead:
String? toastMessage;
...
if (result.source == value) {
toastMessage = mounted ? context.messagesLocalizations.noFormattingChanges : null;
} else {
toastMessage = mounted ? context.messagesLocalizations.formatSuccesful : null;
appModel.sourceCodeController.text = result.source;
}
} catch (error) {
toastMessage = mounted ? context.messagesLocalizations.errorFormatting : null;
appModel.appendLineToConsole('Formatting issue: $error');
}
if (toastMessage?.isNotEmpty ?? false) {
appModel.editorStatus.showToast(toastMessage);
}
Or
if (mounted) {
if (result.source == value) {
appModel.editorStatus
.showToast(context.messagesLocalizations!.noFormattingChanges);
} else {
appModel.editorStatus
.showToast(context.messagesLocalizations!.formatSuccesful);
appModel.sourceCodeController.text = result.source;
}
}
} catch (error) {
if (mounted) {
appModel.editorStatus
.showToast(context.messagesLocalizations!.errorFormatting);
appModel.appendLineToConsole('Formatting issue: $error');
}
}
appModel.editorStatus | ||
// ignore: use_build_context_synchronously | ||
.showToast(context.messagesLocalizations!.compilationFailed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can use a mounted check instead.
Text('Privacy notice'), | ||
SizedBox(width: denseSpacing), | ||
Icon(Icons.launch, size: 16), | ||
Text(MessagesLocalizations.of(context)!.privacyNotice), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When to prefer MessageLocalizations.of(context)
over context.messageLocalizations
?
// ignore: use_build_context_synchronously | ||
_handleSelection(appServices, selection, context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mounted check if (selection != null && mounted)
Fixes #2746.
Currently still depends on
package:flutter_messages
, which is developed by me. We should think about the long-term solution here. Also, RTL languages such as arabic reveal some UI bugs (check this by setting the locale toar
).cc @devoncarew @parlough
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.