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

Example project with search feature implementation #96

Merged
merged 29 commits into from
Sep 21, 2022

Conversation

yendacoder
Copy link
Contributor

Enhancements suggested in #94 and #95

This turned out to be a bigger change than I expected and still, the search example is not that elegant, as for me. Majority of the code is dedicated to handling skin tone popup and it follows the internals of picker quite closely. It does the job, but maybe there are ways to make it tidier.

One of the options I see would be making an independent SearchResults widget that can encapsulate the complexity of this feature, but it would require some sort of config approach that can share some settings with picker and introduce search-only options. I think this goes beyond the scope of this PR.

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 it took my a while to take a look, I'm busy at my full-time job.
I left some comments about things I would like to clarify first 🙇
Thx as usual for your efforts 🙏

))
else
_buildSearchResults(context, emojiSize, cellSize),
_buildSearchBar(context, value.text.isEmpty)
Copy link
Owner

Choose a reason for hiding this comment

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

Currently it's not possible to remove the focus from the searchBar. How about aligning it with Whatsapp keyboard that collapse to SearchResults after pressing on the search-icon already and provides a back button to show the emoji-keyboard again. Just an suggestion since you were not happy with the current example.
Screenshot_20220825-222627

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a back button. It's not exactly as in Whatsapp: they, for example show some common emojis with empty search text. I think it's a good start, but there are so many usability options you can think about which in the end come down to personal preferences. That's why search implementation outside of the library itself makes a lot of sense.

Comment on lines 24 to 22
late final _utils = EmojiPickerInternalUtils();
late final _utils = EmojiPickerUtils();
Copy link
Owner

Choose a reason for hiding this comment

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

Nice 👍 will make it bit easier for people who copy & paste the code for custom View

/// A widget that represents an individual clickable emoji cell.
/// Can have a long pressed listener [onSkinToneDialogRequested] that
/// provides necessary data to show a skin tone popup.
class EmojiCell extends StatelessWidget {
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 thinking you want to extract this logic in order to use it outside of the emoji-keyboard but also inside. In this PR the EmojiCell is only used in your example. Does it make sense to use it internal as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the plan. Somehow, when I was stashing code to move between my workstations, the main changes for the default_emoji_picker_view.dart file got lost. Luckily, I was able to recover it, so it's all good now.

children: [
if (!_isSearchFocused)
IconButton(
onPressed: _searchFocusNode.requestFocus,
Copy link
Owner

@Fintasys Fintasys Sep 19, 2022

Choose a reason for hiding this comment

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

Not a blocker, but I would close the EmojiPicker when the search field is focused. Let me know if you want to change it or not. Then I will merge 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a matter of personal preference, I think. Our internal search method returns empty list for an empty search phrase, so we need a bigger change to replicate WhatsApp behavior. Therefore, to me it makes sense to keep showing full picker UI until user starts typing. This has no effect on the library, though, so I've changed it to show search results whenever search field gets focused.

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.

Beside my comment above it looks fine to me! Glad the code wasn't lost :) Sorry that it took again a while to review your changes 🙏

@Fintasys Fintasys merged commit 91e786e into Fintasys:master Sep 21, 2022
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.

None yet

2 participants