-
Notifications
You must be signed in to change notification settings - Fork 358
refactor(ui)!: re-wrote message actions and modals #2156
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
base: v10.0.0
Are you sure you want to change the base?
Conversation
9f56bed
to
a42c738
Compare
0cde3d3
to
4a8c9ef
Compare
47293cc
to
625524b
Compare
7ce3ee7
to
38156e9
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.
Pull Request Overview
This pull request refactors the message actions and modal system to consolidate duplicated UI components and improve type safety. Key changes include:
- Removal of individual action button widgets in favor of a generic, reusable builder (StreamMessageActionsBuilder) and modal (StreamMessageModal).
- Updates to the retry logic in channel operations and localization tweaks for consistency.
- Various cleanups and removals of legacy context menu and action files to streamline the codebase.
Reviewed Changes
Copilot reviewed 101 out of 101 changed files in this pull request.
Show a summary per file
File | Description |
---|---|
packages/stream_chat_flutter/lib/src/message_actions_modal/* | Removed legacy message action button widgets in favor of a centralized approach. |
packages/stream_chat_flutter/lib/src/message_action/message_actions_builder.dart | Introduced a new builder class to generate message actions based on state and channel capabilities. |
packages/stream_chat_flutter/lib/src/message_action/* | Added new generic message action types and updated related widgets for improved type safety. |
packages/stream_chat_flutter/lib/src/fullscreen_media/full_screen_media_desktop.dart | Updated DownloadMenuItem integration and adjusted menu builder for desktop media. |
packages/stream_chat_flutter/lib/src/localization/translations.dart | Revised error messages to remove line breaks and standardize wording. |
packages/stream_chat_flutter/lib/src/channel/channel.dart | Updated retryMessage logic with enhanced assertions and clearer retry paths. |
packages/stream_chat_flutter/CHANGELOG.md, dart_test.yaml | Updated changelog and test configuration to reflect the refactor. |
class DownloadMenuItem extends StatelessWidget { | ||
/// {@macro streamDownloadMenuItem} | ||
const DownloadMenuItem({ | ||
super.key, | ||
required this.mediaAttachment, | ||
}); |
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 can be private, as it's not used anywhere else. Making it private we prevent it being exposed later by accident.
class DownloadMenuItem extends StatelessWidget { | |
/// {@macro streamDownloadMenuItem} | |
const DownloadMenuItem({ | |
super.key, | |
required this.mediaAttachment, | |
}); | |
class _DownloadMenuItem extends StatelessWidget { | |
/// {@macro streamDownloadMenuItem} | |
const _DownloadMenuItem({ | |
required this.mediaAttachment, | |
}); |
// ignore_for_file: lines_longer_than_80_chars | ||
|
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 you know if the analyzer of pub.dev likes this?
} | ||
|
||
/// Action to show reaction selector for adding reactions to a message | ||
final class SelectReaction extends MessageAction { |
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.
Is there a reason to make all these classes final? Maybe devs see use in extending these and adding their own data? Don't think that would influence our code right?
this.cancelActionTitle = const Text('Cancel'), | ||
this.confirmActionTitle = const Text('Confirm'), |
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 would make these nullable, so the default implementations can use localizations.
} | ||
|
||
/// {@template streamMessageModal} | ||
/// A customized modal widget for displaying message-related content. |
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.
Shouldn't this be "A customizable"?
Alignment.topCenter => CrossAxisAlignment.start, | ||
Alignment.center => CrossAxisAlignment.center, | ||
Alignment.bottomCenter => CrossAxisAlignment.center, |
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.
How does this work? I would expect them to be all center, or start, center, end.
alignment: switch (reverse) { | ||
true => Alignment.centerRight, | ||
false => Alignment.centerLeft, | ||
}, |
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'm wondering if we already should think of RTL support on new code we right. I think it's good to already start thinking about that while building it, but might break some of the layout as we don't really support it anywhere yet.
/// It is not recommended to use this widget directly as it's one of the | ||
/// default widgets used by [StreamMessageWidget.onMessageActions]. | ||
/// {@endtemplate} | ||
class StreamReactionPicker extends StatefulWidget { | ||
class StreamReactionPicker extends StatelessWidget { | ||
/// {@macro streamReactionPicker} | ||
const StreamReactionPicker({ | ||
super.key, | ||
required this.message, | ||
required this.reactionIcons, | ||
this.onReactionPicked, | ||
this.padding = const EdgeInsets.symmetric(horizontal: 8, vertical: 4), | ||
this.scrollable = true, | ||
this.borderRadius = const BorderRadius.all(Radius.circular(24)), | ||
}); | ||
|
||
/// Message to attach the reaction to | ||
final Message message; | ||
/// Creates a [StreamReactionPicker] using the default reaction icons | ||
/// provided by the [StreamChatConfiguration]. | ||
/// | ||
/// This is the recommended way to create a reaction picker | ||
/// as it ensures that the icons are consistent with the rest of the app. |
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.
The docs are a bit conflicting now.
On top of the class you have:
"It is not recommended to use this widget directly"
But for the builder you have "This is the recommended way to create a reaction picker"
I would change the first doc to something like this:
"It is not recommended to use this widget directly, but to use the StreamReactionPicker.builder
for platforms specific optimizations."
void triggerAnimations() async { | ||
for (final animation in iconAnimations) { | ||
if (mounted) animation.start(); | ||
// Add a small delay between the start of each animation. | ||
await Future.delayed(const Duration(milliseconds: 100)); | ||
} | ||
} | ||
|
||
void dismissAnimations() { | ||
for (final animation in iconAnimations) { | ||
animation.stop(); | ||
} | ||
} | ||
|
||
void disposeAnimations() { | ||
for (final animation in iconAnimations) { | ||
animation.dispose(); | ||
} | ||
} |
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.
More of a general comment, but I would make methods like these private. No need to have them public, even though the State class is already private. It's good practice to only make things public that should be public, so we don't have people use them when we don't expect it.
id: 'test-message', | ||
type: MessageType.error, | ||
text: 'This is a test message', | ||
createdAt: DateTime.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.
I wonder if this variable createdAt
can influence the output of the golden test. I would just make this a fixed DateTime value.
Resolves: FLU-79
Description of the pull request
This pull request combines the message actions building logic for desktop and mobile platforms by introducing a class
StreamMessageAction
to provide a reusable builder for message actions. Also introducedStreamMessageModal
for creating message related modals in the SDK.Screenshots / Videos
Screen.Recording.2025-05-13.at.17.11.41.mov