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

[Feature] Add EmojiPickerUtils for advanced usage #38

Merged
merged 13 commits into from Jan 17, 2022

Conversation

zhourengui
Copy link
Contributor

@zhourengui zhourengui commented Jan 4, 2022

close: #37

I encountered a need to search for emojis feature, but I can’t get all emojis, so I think a public method class should be implement for the convenience of expansion.

I think this change should be very useful, for example:

  1. when our package adds a category, we don’t need to modify it in multiple places
  2. want to expand the function of searching emoticons, at this time, we need to get all the expressions, this method does not require the user to reassemble all the expression data.

@Fintasys Fintasys linked an issue Jan 4, 2022 that may be closed by this pull request
@Fintasys
Copy link
Owner

Fintasys commented Jan 4, 2022

Overall I understand what you want to achieve and it does sounds good to me. Beside my comments above I think it would be great if you could also adjust the readme for this change and explain how it can be used.

@zhourengui
Copy link
Contributor Author

Overall I understand what you want to achieve and it does sounds good to me. Beside my comments above I think it would be great if you could also adjust the readme for this change and explain how it can be used.

I have modified it, thank you very much for your feedback.

@zhourengui
Copy link
Contributor Author

I added the addEmojiToRecentlyUsedfang method, because it should be useful when expanding

Copy link
Owner

@Fintasys Fintasys left a comment

Choose a reason for hiding this comment

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

Thx for addressing the previous comments and your efforts! I took a closer look now and found some other issues, place have a look ! 🙇

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/src/emoji_picker.dart Show resolved Hide resolved
lib/src/emoji_picker.dart Outdated Show resolved Hide resolved
lib/src/emoji_picker_utils.dart Outdated Show resolved Hide resolved
@zhourengui
Copy link
Contributor Author

Your point of view is very correct. I didn’t consider the compatibility issue at the beginning. The method I’m exposing now is to get all the available category emoji and all the available emoji entities and search emoticons.

Copy link
Owner

@Fintasys Fintasys left a comment

Choose a reason for hiding this comment

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

Sorry more comments :) Let me know if I should support you 👍 Thanks for your hard work

Comment on lines 26 to 45
static Future<Map<Category, Map<String, String>?>?>
getAvailableCategoryEmoji() async {
if (_availableCategoryEmoji == null) {
// Filter recent category emojis
final categories =
Category.values.where((category) => category != Category.RECENT);

// Get all available emojis from cached
final futures = categories.map((category) async {
final emojis = await restoreFilteredEmojis(category.name);
return emojis;
});

final allAvailableEmojis = await Future.wait(futures);

_availableCategoryEmoji =
Map.fromIterables(categories, allAvailableEmojis);
}
return _availableCategoryEmoji;
}
Copy link
Owner

Choose a reason for hiding this comment

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

In case emoji-picker hasn't been displayed at least once, there are no emojis cached and this method would return an empty Map. Could we maybe extract the logic inside _updateEmojis() instead into another helper class (non-static, mockable for tests) together with restoreFilteredEmojis?

Choose a reason for hiding this comment

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

I also considered this problem at first, but what I thought was that since this tool class should be used only when the pop-up window is used, there may be a problem with my idea.😄

lib/src/emoji_picker_utils.dart Outdated Show resolved Hide resolved
lib/src/emoji_picker_utils.dart Outdated Show resolved Hide resolved
}

/// Add an emoji to recently used list or increase its counter
static Future<List<RecentEmoji>> addEmojiToRecentlyUsed(
Copy link
Owner

Choose a reason for hiding this comment

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

I was wondering what's the use case for this method? How could it be used outside of the Widget?
If it doesn't serve an usage outside of the widget we could also move it to extra helper class

Choose a reason for hiding this comment

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

For example, when searching for expressions, a certain expression is used, and this expression needs to be added to the recent use, so I moved it here

lib/src/emoji_picker.dart Outdated Show resolved Hide resolved
lib/src/emoji_picker.dart Outdated Show resolved Hide resolved
@zhourengui
Copy link
Contributor Author

I have already changed it, I believe this is the best version, and I have tested it locally with version 2.0.1 of flutter, and I have not found any problems for the time being. thank you very much for the feedback these days.😃

@zhourengui
Copy link
Contributor Author

The default value of the recentsLimit parameter of the addEmojiToRecentlyUsed method should be changed to 28. If I don't need to modify the others, please help me change it.

Copy link
Owner

@Fintasys Fintasys left a comment

Choose a reason for hiding this comment

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

Awesome ! we are almost done - just a few little more comments 🙏

lib/src/emoji_picker_extended_utils.dart Outdated Show resolved Hide resolved
lib/src/emoji_picker_extended_utils.dart Outdated Show resolved Hide resolved
lib/src/emoji_picker_extended_utils.dart Outdated Show resolved Hide resolved
lib/src/emoji_picker_internal_utils.dart Outdated Show resolved Hide resolved
lib/src/emoji_picker_internal_utils.dart Show resolved Hide resolved
lib/src/emoji_picker_internal_utils.dart Outdated Show resolved Hide resolved
@zhourengui
Copy link
Contributor Author

It has been revised, although the process is very difficult, but the ending is good.😃

Copy link
Owner

@Fintasys Fintasys left a comment

Choose a reason for hiding this comment

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

I've tested a lot and realized that there were few issues left

  1. search was case sensitive
  2. addEmojiToRecentlyUsed didn't update the widget's recent list
    Took me 2 days to figure out a good way and ended up making a PR. Hope you don't mind and can take a look to my changes. I thought that's easier than explaining what I mean.
    Fix addEmojiToRecentlyUsed zhourengui/emoji_picker_flutter#1

}

return _allAvailableEmojiEntities
.where((emoji) => emoji.name.contains(keyword))
Copy link
Owner

Choose a reason for hiding this comment

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

Search seems to be case-sensitive, I think it would be better if not 😅

Copy link
Owner

@Fintasys Fintasys left a comment

Choose a reason for hiding this comment

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

@zhourengui Sorry for the delay I was busy. Thank you for your hard work, I think it's good to go now. I will improve the documentation as you mentioned in another PR to make it easier for me. Also I have some other changes related to the emoji's that I want to do first before releasing it as a new version, so please be patient a bit more. You can use the master directly in your app if you need the changes asap.

@Fintasys Fintasys changed the title feature emoji picker utils [Feature] Add EmojiPickerUtils for advanced usage Jan 17, 2022
@Fintasys Fintasys merged commit 10d4be5 into Fintasys:master Jan 17, 2022
@zhourengui
Copy link
Contributor Author

@zhourengui Sorry for the delay I was busy. Thank you for your hard work, I think it's good to go now. I will improve the documentation as you mentioned in another PR to make it easier for me. Also I have some other changes related to the emoji's that I want to do first before releasing it as a new version, so please be patient a bit more. You can use the master directly in your app if you need the changes asap.

It's okay, don't worry, I'm currently using a temporary tool class in my project to solve the problem, and when everything is done, I'm going to upgrade.

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.

Search option for emoji?
3 participants