Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add extra asset loaders #654

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

hamed-rezaee
Copy link
Contributor

@hamed-rezaee hamed-rezaee commented Feb 28, 2024

Add multi-asset loader to add more than one asset loader if it is needed, for example, if you want to add package localization to your project.

@hamed-rezaee hamed-rezaee force-pushed the feat/multi_asset_loader branch from bfe6335 to 77ae8fc Compare February 28, 2024 20:12
@bw-flagship
Copy link
Collaborator

Can you elaborate on a usecase? Why do you need multiple asset loaders for package localization?

@hamed-rezaee hamed-rezaee force-pushed the feat/multi_asset_loader branch from 77ae8fc to 3c352dd Compare February 28, 2024 20:18
@hamed-rezaee
Copy link
Contributor Author

Can you elaborate on a usecase? Why do you need multiple asset loaders for package localization?

Hi @bw-flagship

This discussion: #592

and also here is another usecase:

https://stackoverflow.com/questions/78068216/how-can-i-add-a-localizationsdelegate-form-another-package-that-uses-easylocali

I have a package that uses the EasyLocalization package for localization, and I want to add localizationsDelegate of this package to my main project.

I know that I can add something like this with flutter_intl package to pubspec.yaml:

flutter_intl:
  main_locale: en
  enabled: true
  class_name: MyPackageLocalization

And add the MyPackageLocalization.delegate to my localizationsDelegates inside the MaterialApp:

...
MaterialApp(
  localizationsDelegates: [
    ...context.localizationDelegates,
    MyPackageLocalization.delegate,
  ],
  supportedLocales: context.supportedLocales,
...

But, how can I do it with the EasyLocalization package?

@bw-flagship
Copy link
Collaborator

@hamed-rezaee thanks for explaining!
I understand the purpose of the PR an since the discussion has at least a few upvotes, it seems to be a useful feature.

Some things prevent me from approving this pr in the current state:

  • The breaking change seems unnecessary. You can optionally accept a single asset loader in the constructor and pass it to the list. In v4.0 we could make the breaking change then
  • Instead of (await Future.wait(loaderFutures)).othermethods(), I think await Future.wait(loaderFutures).then((x) => othermethods()) would be more readable
  • Let's avoid the dependency to collection if possible. Each dependency we introduce might cause problems for consumers of the library
  • There seem to be unnecessary formatting changes which make the PR hard to review
  • We need unittests for this new functionality and for the new util

@hamed-rezaee
Copy link
Contributor Author

@hamed-rezaee thanks for explaining! I understand the purpose of the PR an since the discussion has at least a few upvotes, it seems to be a useful feature.

Some things prevent me from approving this pr in the current state:

  • The breaking change seems unnecessary. You can optionally accept a single asset loader in the constructor and pass it to the list. In v4.0 we could make the breaking change then
  • Instead of (await Future.wait(loaderFutures)).othermethods(), I think await Future.wait(loaderFutures).then((x) => othermethods()) would be more readable
  • Let's avoid the dependency to collection if possible. Each dependency we introduce might cause problems for consumers of the library
  • There seem to be unnecessary formatting changes which make the PR hard to review
  • We need unittests for this new functionality and for the new util

@bw-flagship I will apply the suggested changes and update the PR asap. 👍🏻

@hamed-rezaee hamed-rezaee marked this pull request as draft February 29, 2024 19:38
@hamed-rezaee hamed-rezaee force-pushed the feat/multi_asset_loader branch from 41d4f2c to 74f124b Compare February 29, 2024 20:40
@hamed-rezaee hamed-rezaee changed the title add multi-asset loader add extra asset loaders Feb 29, 2024
@hamed-rezaee hamed-rezaee marked this pull request as ready for review February 29, 2024 20:42
@hamed-rezaee
Copy link
Contributor Author

Hey @bw-flagship this PR is ready for review.

Copy link
Collaborator

@bw-flagship bw-flagship left a comment

Choose a reason for hiding this comment

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

This looks very good! Some minor findings

lib/src/easy_localization_controller.dart Outdated Show resolved Hide resolved
lib/src/utils.dart Show resolved Hide resolved
lib/src/easy_localization_controller.dart Outdated Show resolved Hide resolved
@hamed-rezaee hamed-rezaee requested a review from bw-flagship March 1, 2024 11:43
@bw-flagship bw-flagship merged commit 63a3974 into aissat:develop Mar 1, 2024
4 checks passed
@hamed-rezaee hamed-rezaee deleted the feat/multi_asset_loader branch March 1, 2024 14:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants