-
-
Notifications
You must be signed in to change notification settings - Fork 21
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 reply messages preview #2430
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2430 +/- ##
==========================================
+ Coverage 27.60% 27.72% +0.12%
==========================================
Files 651 657 +6
Lines 43921 44743 +822
==========================================
+ Hits 12125 12407 +282
- Misses 31796 32336 +540
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
UI/UX wise looks good but I think we can do better code wise. In particular around two aspects of the way this is being implemented: a) better separation of concerns - it's not metadata if it is purely about the way the same object is rendered differently depending on context (I marked that situation)
b) I think the bubble should not become too intelligent, but be a dump widget that other higher level objects use to render their widgets (or not). For reusability we can have common code (e.g. a Reply-To-Widget-Builder) in a (abstract) widget that others mix in or can inherit from. I think that is better, cleaner separation and leads to bigger reusability than tweaking a "metadata" object you pass around to maybe show a bubble or not...
app/lib/features/chat_ng/providers/notifiers/reply_messages_notifier.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/providers/notifiers/reply_messages_notifier.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/providers/notifiers/reply_messages_notifier.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/providers/notifiers/reply_messages_notifier.dart
Outdated
Show resolved
Hide resolved
app/lib/features/chat_ng/widgets/events/reply_original_event.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.
nice!
build on top of #2426 as it brings core structural improvements to messages UI, this includes reply messages preview along with proper automatic provider updates on
repliedTo
message ids whenever valid reference gets available. Also supports nice error UI handling.Preview:
showcases the replied to content UI and notice how the original event gets available on server fetch when paginating:
Screen.Recording.2024-12-17.at.16.18.03.mov