-
Notifications
You must be signed in to change notification settings - Fork 296
Use shared recent emoji reactions from account data #5402
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
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
...ment/android/features/messages/impl/timeline/components/customreaction/picker/EmojiPicker.kt
Show resolved
Hide resolved
28d8581
to
434b80e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested, but one first comment
persistentListOf("👍️", "👎️", "🔥", "❤️", "👏") | ||
} else { | ||
recentEmojis.take(50) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a product decision on this?
I think it can be improved, because before the user sends a very first reaction, they will see the 5 default emojis, and after they send a first one, it will be the only one in the list which is weird since it can be any random one.
Maybe we should always display the 5 default emojis after the list of the recent ones?
recentEmojis.take(50) + listOf("👍️", "👎️", "🔥", "❤️", "👏").filter { it !in recentEmojis }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a product decision on this?
Not really, I just kept the previous suggested emoji reactions for the default values.
Maybe we should always display the 5 default emojis after the list of the recent ones?
That's one option, but then it breaks the 1:1 match with EW this PR wanted to provide.
@mxandreas any opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I fully understand everything here (if it easy, perhaps re-state the question). But generally consistency with EW is key, unless EW's behavior is known to be problematic and should be improved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this message was for Jorge)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I fully understand everything here (if it easy, perhaps re-state the question). But generally consistency with EW is key, unless EW's behavior is known to be problematic and should be improved.
When we don't have any recently used emojis, we suggest using any of these:
👍️ 👎️ 🔥 ❤️ 👏
But then, once an emoji has been used (i.e. 🔹), the user will see just this for the suggested emoji reactions:
🔹
Which is a bit odd, but then they'd also see just that single suggested emoji in EW if I'm not mistaken. @bmarty proposes having this after the 1st emoji is used:
🔹 👍️ 👎️ 🔥 ❤️ 👏
But then it wouldn't match EW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For @mxandreas if I understand correctly the PR, when long clicking on any message, on an account that has not sent any reaction, user will see this:

and once a reaction 💩 will be sent, they will see this:

I propose that they always see the default reaction:

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true we can't really match EW 1:1 since we're using their 'quick actions' as the suggested ones, and then we'd want to replace them / add the recent ones to those, which on EW are 2 different sections. Maybe @bmarty 's suggestion is the best option. WDYT @mxandreas ? - sorry for pinging you so much on this topic, btw 😅.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, no :-). Imagine I used a few popular emojis, then I would see:
👍️ 🔥 👍️ 👎️ 🔥 ❤️ 👏
and other weird duplications, which looks incredibly clumsy.
Proposal: If getRecentEmojis is empty, prime the list once with setRecentEmojis.addEmoji('👍️') (or whatever the exact API is) and do this for 3-4 emojis. One could do this once on startup and be done with it or each time the emoji list is shown.
Will shut up now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end we're doing a mix: apparently, iOS was already using a concatenation of the suggested emojis + the recent emojis provided by the OS, removing duplicates. We'll do the same here, since there were some users who complained about the quick reactions changing too frequently with the recent emojis and they tapped the wrong emojis by muscle memory.
bd26c6d
to
8f1a629
Compare
There were some issues with recording screenshots, let's see if the latest changes fix them 🤞 . |
- Add `AddRecentEmoji` and `GetRecentEmojis` use cases to avoid injecting the whole `MatrixClient` for just one of these operations. - Update the UI and logic of the emoji picker and message context menu to include the recent emojis. - Add `CoroutineDispatchers.Default` with the defaults coroutines to use in the app for ease of use.
7f744f1
to
db1e2f3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5402 +/- ##
===========================================
+ Coverage 80.37% 80.38% +0.01%
===========================================
Files 2282 2284 +2
Lines 63071 63150 +79
Branches 7887 7889 +2
===========================================
+ Hits 50694 50764 +70
- Misses 9464 9475 +11
+ Partials 2913 2911 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
89a1bfd
to
b686f10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
.../src/main/kotlin/io/element/android/features/messages/impl/actionlist/ActionListPresenter.kt
Show resolved
Hide resolved
...android/features/messages/impl/timeline/components/customreaction/CustomReactionPresenter.kt
Outdated
Show resolved
Hide resolved
|
* Use shared recent emoji reactions from account data - Add `AddRecentEmoji` and `GetRecentEmojis` use cases to avoid injecting the whole `MatrixClient` for just one of these operations. - Update the UI and logic of the emoji picker and message context menu to include the recent emojis. - Add `CoroutineDispatchers.Default` with the defaults coroutines to use in the app for ease of use. * Instead of replacing suggested emojis, concatenate recent ones removing duplicates * Update screenshots --------- Co-authored-by: ElementBot <[email protected]>
Content
AddRecentEmoji
andGetRecentEmojis
use cases to avoid injecting the wholeMatrixClient
for just one of these operations.CoroutineDispatchers.Default
with the defaults coroutines to use in the app for ease of use.Motivation and context
Iterate on #5255, continue integrating the work done for the hackathon around temojis.
Screenshots / GIFs
Then on Element Web for the same account:
Tests
Tested devices
Checklist