Fintasys / emoji_picker_flutter

A Flutter package that provides an Emoji picker widget with 1500+ emojis in 8 categories.
MIT License
170 stars 127 forks source link

Performance and usability improvements #90

Closed yendacoder closed 2 years ago

yendacoder commented 2 years ago

Hi,

I went quite far with this PR. Let me know if you feel like this deviates too much from your original vision, I'll publish my fork under a different name. I wanted to address two issues before using this library in my project: performance (scroll lag) and compatibility (icons support, specifically on Android: some icons while technically exist in system fonts, don't have a colored version) emojis

So, here is the list of my changes (at least noteworthy ones):

Performace-related

Code simplification

Compatibility

General usability

Preview of the result.

emojis2

Fintasys commented 2 years ago

@yendacoder wow, thank you for your huge contribution. I would have preferred each feature in its own PR for making it easier to review and judge if it should become part of the package. I know this can be difficult sometimes but it makes the live of maintainer so much easier and speed up things. Nevertheless I will try to review your PR, check your changes and will let you know about it. Also it probably conflicts with changes I had in my pipeline. Please give me some time to check it 🙇

yendacoder commented 2 years ago

Hi @Fintasys Yes, it should have been at least two PR's (some changes are quite interconnected, but, of course, not all of them), but I've made a lot of changes rather spontaneously. Sorry for that. I saw that your issues on the web branch (if it is the one you were referring to) are mostly related to custom fonts, which I think are working well with these changes. So, if we can get through review with this, updating the library for web support shouldn't be that difficult.

yendacoder commented 2 years ago

Thanks for the thorough review. I'll address some of the issues tomorrow, you had some excellent catches. The CI issue was caused by that koala PR indeed. Should be good now. I think it's good to make a major version out of this because of progress color setting deprecation (that would be a breaking change). Otherwise, it should be backwards compatible, even if we bring back caching.

yendacoder commented 2 years ago

I think that we are almost there with this PR. Before you make a new version out of it, I'd like to make another one that possibly can be included in the new version as well. It concerns search feature. I think that search itself can be outside of picker itself (otherwise, there will be too many settings with texts and layouts), so I'm planning to make another example dedicated to search specifically. But to make it more manageable, I think the library should expose a widget for emoji cells that host can use to display search results.

Let me know what you think.

Fintasys commented 2 years ago

I also think that the PR is good to be merged. I will also work on improving the documentation, the examples, updating version etc. I'm also curious about the changes you want to do to the search function! I would appreciate if you might can line out quickly some details in an issue first before starting a PR.

It seems CI(analyse) is not happy with you current code changes :D After addressing this I will merge this PR into master 👍

yendacoder commented 2 years ago

The lint issues have been fixed.

I've also created 2 issues to outline the next changes I have in mind.

I think that this PR can be merged.