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

Add localizations #391

Merged
merged 11 commits into from
Aug 10, 2024
Merged

Add localizations #391

merged 11 commits into from
Aug 10, 2024

Conversation

maelchiotti
Copy link
Contributor

Add default localizations with a delegate.

@maelchiotti maelchiotti marked this pull request as draft August 4, 2024 12:57
@Amir-P
Copy link
Member

Amir-P commented Aug 4, 2024

I can see that it's marked as a draft. Let me know when it's ready for review.

@maelchiotti
Copy link
Contributor Author

Indeed, I didn't add localizations for all the strings in the package, that's the only thing that's missing (and fixing tests). I'll tag you when I'm done.

@maelchiotti maelchiotti marked this pull request as ready for review August 9, 2024 13:09
@maelchiotti
Copy link
Contributor Author

I've added the other strings, with translations in english and french. Now other languages can be supported just by adding their ARB files.

I've scrolled through all the code to find strings to translate so I hope I didn't miss any. I've also fixed the tests.

So the PR is ready for review now!

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 7 lines in your changes missing coverage. Please review.

Project coverage is 87.90%. Comparing base (9c5818d) to head (0f9b6fb).
Report is 2 commits behind head on master.

Files Patch % Lines
packages/fleather/lib/src/widgets/link.dart 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
- Coverage   87.93%   87.90%   -0.03%     
==========================================
  Files          61       62       +1     
  Lines       10299    10329      +30     
==========================================
+ Hits         9056     9080      +24     
- Misses       1243     1249       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Amir-P
Copy link
Member

Amir-P commented Aug 9, 2024

Are you ok with me pushing a commit on this branch? I had too many improvements in mind that would be a bit time consuming to write and also for you to apply. @maelchiotti

@maelchiotti
Copy link
Contributor Author

Yes of course you can!

@Amir-P
Copy link
Member

Amir-P commented Aug 9, 2024

Can you please take a look and let me know what you think about the changes? It also would have been better if we could completely ignore generated files from the plugin, but looks like those files don't get generated when flutter pub get called on the dependent project. So we have to include them. @maelchiotti

@maelchiotti
Copy link
Contributor Author

The changes look good to me!

@Amir-P Amir-P requested a review from amantoux August 9, 2024 20:34
@amantoux
Copy link
Member

amantoux commented Aug 9, 2024

do you mind if I commit a change to the repo to avoid having the **.g.dart files in the coverage stats?

@amantoux
Copy link
Member

amantoux commented Aug 10, 2024

I've added the other strings, with translations in english and french

Cocorico :)

Copy link
Member

@amantoux amantoux left a comment

Choose a reason for hiding this comment

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

Great to see Fleather in French!
General, I think we should stick to Flutter team practice and put l10n under lib
I have a few comments on the imports

@@ -1,12 +1,9 @@
import 'dart:async';

import 'package:fleather/src/widgets/editor.dart';
import 'package:fleather/fleather.dart';
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid importing via package for internal files, can you please use relative path?

@@ -1,12 +1,9 @@
import 'dart:async';

import 'package:fleather/src/widgets/editor.dart';
import 'package:fleather/fleather.dart';
import 'package:fleather/l10n/l10n.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@@ -1,7 +1,8 @@
import 'package:fleather/fleather.dart';
Copy link
Member

Choose a reason for hiding this comment

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

use relative path for import?

@@ -1,7 +1,8 @@
import 'package:fleather/fleather.dart';
import 'package:fleather/l10n/l10n.dart';
Copy link
Member

Choose a reason for hiding this comment

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

use relative path for import?

Copy link
Member

@amantoux amantoux left a comment

Choose a reason for hiding this comment

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

LGTM
THanks @maelchiotti & @Amir-P

@amantoux amantoux merged commit e7c7a9a into fleather-editor:master Aug 10, 2024
2 of 3 checks passed
@maelchiotti
Copy link
Contributor Author

Feel free to tag me if you add a new string anytime, so I can translate it in french directly 😊

@amantoux
Copy link
Member

Feel free to tag me if you add a new string anytime, so I can translate it in french directly 😊

It shouldn't be a problem, I'm French too 🥖

@maelchiotti
Copy link
Contributor Author

Aha awesome 🥖🥖

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.

3 participants