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

Internationalization/localization #374

Closed
Sjonnie2nd opened this issue Jul 4, 2024 · 18 comments
Closed

Internationalization/localization #374

Sjonnie2nd opened this issue Jul 4, 2024 · 18 comments
Labels
enhancement New feature or request

Comments

@Sjonnie2nd
Copy link
Contributor

At first. As a developer, wrestling for 20 years with things like contenteditable htmleditors, im very much impressed by this package.
It gives a simple, fast and robust feeling working with it.

Testing the editor in my project, and digging through te code on this repo I noticed that the vocabulary in the user interface looks mostly hard coded: "Automatic", "No color", "Normal", "Heading 1/6", "Copy", "Paste", "Select all", "Paste your url", "Apply" etc..

Am I missing something, or is there no generic builtin functionality for internationalization/localization?

Thanks in advance.

@amantoux amantoux added the enhancement New feature or request label Jul 4, 2024
@amantoux
Copy link
Member

amantoux commented Jul 4, 2024

Thank you for the warm comments, it is much appreciated.

Regarding the internationalization, indeed we have not tackled this yet but we would be happy to review a PR if you want to contribute.

In the meanwhile, we will keep it in our backlog

@Sjonnie2nd
Copy link
Contributor Author

I'd be very pleased to contribute. But I do not have any experience (yet) in creating PR's.

A simple approach could be a class in the root of the widget with default English values:

class Localization {
  const Localization({
    super.key,
    this.copy = 'Copy',
    this.paste = 'Paste',
    ..
  });
  final String copy;
  final String paste;
  ..
}

Then if for example the developer uses the localization package "flutter_localizations" it could be simply bound by:

  AppLocalizations l = AppLocalizations.of(context)!;

  return FleatherEditor(
    localization: Localization(
      copy: l.copy,
      paste: l.paste,
    ),
    ..
  );

The advantage would be that this package doesn't have to be aware of all the existing languages in the world.

Some things to consider:

  • All used words/sentences have to be collected and defined within the class (only once). But I noticed the number of used words is relatively small. I'd say lets keep it that way. The lesser the better.
  • All places where these words are found have to be linked to the class.
  • For changes, additions within this package the developer has to be aware of the used localization techique.

Any thoughts, suggestions?

@Amir-P
Copy link
Member

Amir-P commented Jul 10, 2024

@Sjonnie2nd Thanks for the proposal. That looks promising and I think it's the way to go. Could you please open a PR if you're interested?

@maelchiotti
Copy link
Contributor

I would benefit from this improvement, and it doesn't seem too hard. I could work on this @Amir-P. Unless you want to use this as an opportunity to discover how pull requests work, @Sjonnie2nd?

@Sjonnie2nd
Copy link
Contributor Author

@maelchiotti I certainly want to encourage you to pick this up.

As said I have no experience in doing PR's.
But also my experience with Fleather is very limited for now. Things like how to inject the localized words/sentences in the right places. For example how to replace "Automatic" and "No color" in the toolbar. Pass it in the basic constructor? Do some late binding through the controller? Or in some way through the BuildContext? No idea..

If I can help in some way, please let me know..

@amantoux
Copy link
Member

@maelchiotti All yours! 😉
Let us know if you need any help

@maelchiotti
Copy link
Contributor

I'll start looking into it when I can!

@Sjonnie2nd
Copy link
Contributor Author

@maelchiotti A view things to help you a little:

@maelchiotti
Copy link
Contributor

Thank you!

  • Yes indeed I just tried it with my app, and they correctly get translated automatically, that's great.
  • I guess my changes will cover his improvements since they will be global to the whole package.

@maelchiotti
Copy link
Contributor

maelchiotti commented Aug 2, 2024

I've started to work on this, see #388.

I've straight up copied the FleatherTheme class to make another InheritedWidget that holds the localizations. So far, I've included those for the dialog to add a link and for the toolbar headings formatters.

The new FleatherLocalizations widget can then be added to the widget tree, above both the editor and the toolbar:

FleatherLocalizations(
  data: FleatherLocalizationsData(...),
  child: Column(
    children: [
      FleatherEditor(controller: controller),
      FleatherToolbar.basic(controller: controller),
    ],
  ),
)

If you think this is the right way to do it, then I will go ahead replace all the strings I can find.

@Sjonnie2nd
Copy link
Contributor Author

I'm not sure if with "you" you mean me :) but..
As I see, an optional surrounding localization widget with default fallbacks internally looks very great!!
And.. the way to go!

@maelchiotti
Copy link
Contributor

maelchiotti commented Aug 3, 2024

Yeah, it's great to have your feedback as a user, but indeed I'm mainly awaiting for the approval of the maintainers. Glad you think it's a good solution!

@Amir-P
Copy link
Member

Amir-P commented Aug 3, 2024

Hey @maelchiotti. I think what you did so far looks great. But it might be better to take the Flutter's Cupertino and Material widgets localization approach. Can you take a look and see if you can make the changes? I suggest looking at DefaultMaterialLocalizations, GlobalMaterialLocalizations, _MaterialLocalizationsDelegate and LocalizationsDelegate.
Beside something like FleatherLocalizations.delegate we need to provide an easy way for user to provide it's own supported locale and data. Let me know if you need any help. We can co-author it if you want.

@maelchiotti
Copy link
Contributor

I've looked into it @Amir-P, thank you for your input. Indeed it would be a much better and even easier solution. So I'll start working on providing localizations directly from the package with a delegate, as that would already be a big improvement, and I believe what most users would stick to.

However, allowing users to provide their own translations, although a related issue, could be considered a different feature? A useful one as the PR to change the headings labels shows, but it may be easier to tackle it later, and just focus on providing "default" localizations for now.

@maelchiotti
Copy link
Contributor

I've closed the old PR as the code completely changed, and I've opened a new one in #391 that now uses a delegate. I've added english and french localizations for the same strings as before. All the user needs to do is add the FleatherLocalizations.delegate to his app. I've tested it and it works great.

@Amir-P
Copy link
Member

Amir-P commented Aug 4, 2024

Thank you for taking the time and working on it! I will take a look at it and will continue the code review there. Regarding the extensibility, I believe we need to provide a way for users to use their own translations. Otherwise it wouldn't be a fix for the issue at hand.
However, if you just want to add the building blocks and foundations for localization in Fleather, we can move the PR forward and add public APIs for customizing translations later. @maelchiotti

@maelchiotti
Copy link
Contributor

Yes, I think adding those foundations will be a great start. Then later I'll see if I want to work on enabling custom localization - I wouldn't use them myself so I don't know if I would take the time to do it.

@amantoux
Copy link
Member

Closed with #391

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants