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

Reaction emoji search / Sending freeform reactions #2892

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

frebib
Copy link
Contributor

@frebib frebib commented May 21, 2024

Adds a search field to the reaction emoji picker, and also allows sending freeform reactions.

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Motivation and context

#1611

This is a pretty opinionated patch, and is likely not in a mergeable state. It's mostly provided as a proof-of-concept/starting point. I've been using this for a few months now and I cannot live without it.

Notable bugs in this implementation are the bottom sheet stalling briefly when the keyboard opens. There is an open bug ticket https://issuetracker.google.com/issues/323792322 and no resolution yet. This happens because the ModalBottomSheet has imePadding() unconditionally applied. A fix would be to use a different bottom sheet implementation without that.

Screenshots / GIFs

Coming soon

Tests

😰

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

Copy link
Contributor

Thank you for your contribution! Here are a few things to check in the PR to ensure it's reviewed as quickly as possible:

  • Your branch should be based on origin/develop, at least when it was created.
  • There is a changelog entry in the changelog.d folder with the Towncrier format.
  • The test pass locally running ./gradlew test.
  • The code quality check suite pass locally running ./gradlew runQualityChecks.
  • If you modified anything related to the UI, including previews, you'll have to run the Record screenshots GH action in your forked repo: that will generate compatible new screenshots. However, given Github Actions limitations, it will prevent the CI from running temporarily, until you upload a new commit after that one. To do so, just pull the latest changes and push an empty commit.

ChelseaDH and others added 2 commits May 21, 2024 21:02
Fixes: element-hq#1611
Co-Authored-by: Chelsea Hilditch <[email protected]>
Signed-off-by: Joe Groocock <[email protected]>
@jmartinesp
Copy link
Member

FWIW I am personally very happy to see this since I was working on a simplified version of this on my spare time, just to know how difficult it'd be to implement (the answer being:, the logic is quite easy, the UX not so much). However, since this is a change in design and it'll need to be added on iOS too we need to get it through the product & design team first. Could you upload some screenshots of what the emoji picker looks like with these changes?

Also, for new changes that include changing UI it would be nice to have some issue with a proposal first so we can validate the approach so contributors don't waste their time on PRs that won't be merged or re-writing some screen or logic again and again.

@jmartinesp jmartinesp linked an issue May 22, 2024 that may be closed by this pull request
@jmartinesp
Copy link
Member

A fix would be to use a different bottom sheet implementation without that.

About this: we're using our own fork of the non-modal bottom sheet component for the RTE in text formatting mode because we found lots of issues with focus and scrolling that hadn't been fixed in months. We're not looking forward to forking yet another component, but it's something we could do, if necessary.

@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label May 22, 2024
@amshakal amshakal marked this pull request as ready for review June 10, 2024 11:44
@amshakal amshakal requested a review from a team as a code owner June 10, 2024 11:44
@amshakal amshakal requested review from jmartinesp and removed request for a team June 10, 2024 11:44
@jmartinesp jmartinesp marked this pull request as draft June 10, 2024 11:46
@amshakal
Copy link

This looks good to me and matches well with our design language. Thank you for your contribution!

@CLAassistant
Copy link

CLAassistant commented Sep 9, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ frebib
❌ ChelseaDH
You have signed the CLA already but the status is still pending? Let us recheck it.

@Aranjedeath
Copy link

I would love to see this added to Element X, and I think I only see a sign-off missing. Can we get that done? Thank you so much for contributing this, it is a major user-facing pain point with Element X :)

@jmartinesp
Copy link
Member

This is still an issue for the search part: https://issuetracker.google.com/issues/324934884

In fact, the first report was added in some other issue almost a year ago (we can have an anniversary party!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reaction emoji picker has no search
7 participants