Fintasys / emoji_picker_flutter

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

For example app make number of emoji columns dynamic based on screen width #134

Closed timmaffett closed 1 year ago

timmaffett commented 1 year ago

This change makes the number of columns for the emoji picker dynamic based on screen width. When running the example on windows I was surpised to see this ridiculous result:

emoji_picker_flutter_example 4_30_2023 12_40_14 PM

Here instead is a more expected behavior for an emoji picker (this is the example with this PR added):

emoji_picker_flutter_example 4_30_2023 12_41_41 PM

For clarity I split out all of the emoji size related calculations to the top of the build method to better illustrate to the users what is going on.

For mobile versions the number of columns remains a reasonable number (for Pixel3 emulator it remains 7 as it was hard coded, for more modern phones the number of emoji columns increases as one would expect).

Thanks for the nice package!

Fintasys commented 1 year ago

@timmaffett Thanks for your improvements to the example ! Code-wise it looks good to me, but I would like to avoid too much complexity in the basic example. I would prefer to create an extra example e.g. main-dynamic-columns.dart in the example folder. What do you think ?

timmaffett commented 1 year ago

@Fintasys I have moved the dynamic column sizing to it's own file.

timmaffett commented 1 year ago

I was just trying to illustrate it was a 9 pixel margin on both sides of the emoji. The compiler does the math so it is identical to writing 18. Maybe I need to explain that in a comment or just put 18.. 😜

On Sun, Jun 4, 2023, 6:01 AM Stefan Humm @.***> wrote:

@.**** commented on this pull request.

In example/lib/main-dynamic-columns.dart https://github.com/Fintasys/emoji_picker_flutter/pull/134#discussion_r1216676697 :

+} + +class _MyAppState extends State {

  • final TextEditingController _controller = TextEditingController();
  • bool emojiShowing = false;
  • @override
  • void dispose() {
  • _controller.dispose();
  • super.dispose();
  • }
  • @override
  • Widget build(BuildContext context) {
  • final screenSize = MediaQuery.of(context).size;
  • final emojiPadding = 9*2;

Any reason for not just writing 18 ? 🤔

— Reply to this email directly, view it on GitHub https://github.com/Fintasys/emoji_picker_flutter/pull/134#pullrequestreview-1461176044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3YUSLVVKELO4DUVP6HDPLXJSBLBANCNFSM6AAAAAAXRD4OFA . You are receiving this because you were mentioned.Message ID: @.***>

Fintasys commented 1 year ago

Comment makes more sense. After that I gonna merge, thanks for the work! Seems also Github actions are failing. Please run flutter format lib test example before pushing

timmaffett commented 1 year ago

done and I ran dart format as well.