-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
Chat-NG Reactions #2498
Chat-NG Reactions #2498
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2498 +/- ##
==========================================
- Coverage 29.48% 28.92% -0.57%
==========================================
Files 680 687 +7
Lines 45488 45695 +207
==========================================
- Hits 13413 13218 -195
- Misses 32075 32477 +402
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -113,19 +119,21 @@ class ChatBubble extends StatelessWidget { | |||
const SizedBox(height: 10), | |||
], | |||
child, | |||
if (isEdited) ...[ |
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.
moved Edited
indicator inside bubble for better UX.
show ReactionRecord; | ||
import 'package:flutter/material.dart'; | ||
|
||
class ReactionChipsWidget extends StatelessWidget { |
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 part is logically same and pasted from legacy but more linear and structured.
import 'package:flutter/material.dart'; | ||
import 'package:flutter_riverpod/flutter_riverpod.dart'; | ||
|
||
class ReactionDetailsSheet extends ConsumerStatefulWidget { |
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 is also same logic-wise but have been split because of its complexity.
|
||
final _log = Logger('a3::chat::reactions_selector_row'); | ||
|
||
class ReactionSelectorRow extends ConsumerWidget { |
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 is same as EmojiRow
widget logic-wise but have been more degraded for simplicity and structured.
Just from the UI I notice the following:
|
|
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.
Taking bit of time to review and understand the things. Added few comments related to suggestion/queries.
Here are some more UI Feedback.
-
Reaction Details : I don't see which tab is selected. Also as @gnunicorn mentioned it will be good if we show User avatar and Display name here. And overall UI-UX can be improve for this dialog.
-
Reaction Chip Design : Chip design looks different for send and receiver messages. Also ReactionList overlaying/position and overall design can be improve by taking references of well-known social media app.
app/lib/features/chat_ng/actions/reaction_selection_action.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_selector.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_selector.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_detail_sheet.dart
Outdated
Show resolved
Hide resolved
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.
Generally looks fine to me, but there's a few things to be addressed.
app/lib/features/chat_ng/widgets/reactions/reaction_detail_sheet.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_detail_sheet.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_detail_sheet.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_detail_sheet.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_detail_sheet.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/reactions/reaction_selector.dart
Outdated
Show resolved
Hide resolved
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.
You nicely split the reactions up into their own subfolder ... but I wonder: are these chat specific? We do (later) want users to be able to react to all types of things (pins, comments, tasks and tasklists), doesn't it make sense to put these out into their own features folder already?
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.
You nicely split the reactions up into their own subfolder ... but I wonder: are these chat specific? We do (later) want users to be able to react to all types of things (pins, comments, tasks and tasklists), doesn't it make sense to put these out into their own features folder already?
So, reaction toggling is operated through timelineStreamProvider
which is from Convo
object. Do we have support of reaction API on other room objects? If so, I can maybe change this action a bit and separate it neatly as singleton.
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.
keep it for the future, @gtalha07 . leave it as is.
app/lib/features/chat_ng/widgets/reactions/reaction_selector.dart
Outdated
Show resolved
Hide resolved
@@ -139,3 +141,18 @@ final repliedToMsgProvider = AsyncNotifierProvider.autoDispose | |||
.family<RepliedToMessageNotifier, RepliedToMsgState, RoomMsgId>(() { | |||
return RepliedToMessageNotifier(); | |||
}); | |||
|
|||
final messageReactionsProvider = StateProvider.autoDispose |
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 wonder if that should be per item object or rather be bound to the roomId+eventId combination ... not sure yet.
Description:
Edited
indicator in the message bubble body.Preview/Screenshots:
Video showcasing reactions view (on long-press message):
Screen.Recording.2025-01-16.at.18.20.44.mov
Emoji Selector view on messages:
Reaction UI on messages:
Detailed reaction view (on long press reaction UI) Updated: