Edit-message (6/n): Misc. preparation for edit-message feature #1483
Conversation
lib/widgets/action_sheet.dart
Outdated
| @@ -562,7 +562,7 @@ void showMessageActionSheet({required BuildContext context, required Message mes | |||
| // So we rely on the fact that isComposeBoxOffered for any given message list | |||
| // will be constant through the page's life. | |||
| final messageListPage = MessageListPage.ancestorOf(pageContext); | |||
| final isComposeBoxOffered = messageListPage.composeBoxController != null; | |||
| final isComposeBoxOffered = messageListPage.composeBoxState?.controller != null; | |||
There was a problem hiding this comment.
Could this just be
| final isComposeBoxOffered = messageListPage.composeBoxState?.controller != null; | |
| final isComposeBoxOffered = messageListPage.composeBoxState != null; |
As the dartdoc of composeBoxState states:
/// The [ComposeBoxState] for this [MessageListPage]'s compose box,
/// if this [MessageListPage] offers a compose box and it has mounted,
/// else null.
ComposeBoxState? get composeBoxState;Checking composeBoxState?.controller != null seems a bit confusing use of the API to me.
test/widgets/message_list_test.dart
Outdated
| final stream = eg.stream(); | ||
| await setupMessageListPage(tester, narrow: ChannelNarrow(stream.streamId), | ||
| streams: [stream], | ||
| messages: [eg.streamMessage(stream: stream, content: "<p>a message</p>")]); | ||
| final state = MessageListPage.ancestorOf(tester.element(find.text("a message"))); | ||
| check(state.composeBoxController).isNotNull(); | ||
| check(state.composeBoxState?.controller).isNotNull(); |
There was a problem hiding this comment.
Similar to the earlier comment on a null-check, I feel that checking if composeBoxState?.controller is null will make more sense if the controller is nullable. The updated contract seems to be: as long as composeBoxState is present, there is a compose box and vice versa.
There was a problem hiding this comment.
Yeah; I think what happened is I did something like a copy-paste and didn't make this refinement from there :)
| @@ -906,7 +858,7 @@ class CopyMessageTextButton extends MessageActionSheetMenuItemButton { | |||
|
|
|||
There was a problem hiding this comment.
nit: perhaps update the reference here?
@override void onPressed() async {
// This action doesn't show request progress.
// But hopefully it won't take long at all; and
// fetchRawContentWithFeedback has a TODO for giving feedback if it does.
chrisbobbe
force-pushed
the
pr-edit-message-6
branch
from
c799aed to
8e330b2
Compare
|
Thanks for the review! Revision pushed. |
PIG208
added
integration review
|
The updates LGTM. Thanks! Marking for Greg's review. |
lib/widgets/content.dart
Outdated
| return InheritedMessage(message: message, | ||
| child: DefaultTextStyle( | ||
| style: ContentTheme.of(context).textStylePlainParagraph, | ||
| child: switch (content) { | ||
| ZulipContent() => BlockContentList(nodes: content.nodes), | ||
| PollContent() => PollWidget(messageId: message.id, poll: content.poll), | ||
| })); | ||
| child: child)); |
There was a problem hiding this comment.
This part might become clearer in the upcoming PR that will use this change. But I'm not sure that here in MessageContent is the right place to implement this behavior:
content [nfc]: Pull out `child` variable in MessageContent.build
For the edit-message feature, the Figma fades message content during
the loading state:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4026-8563&m=dev
This refactor prepares for that.
It feels like it might more naturally live in the parent widget.
(Which could do it just as well, right? I imagine the fading doesn't interact with the DefaultTextStyle or the InheritedMessage.)
There was a problem hiding this comment.
Indeed; yeah, I'll drop this commit.
Taken from Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=56-11686&m=dev There was a different "edit" icon on the Icons page: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=557-24893&m=dev but I chose this one because it's the one used in the design for the edit-message button: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3988-35243&m=dev
Soon we'll want a method for starting a message-edit session, and it'll make sense to put it on ComposeBoxState rather than ComposeBoxController; one reason is that it'll need UI code to show an error dialog. With this change, the ComposeBoxState -- and so that new method -- will be accessible in the message-list widget code, where we open the message action sheet from.
chrisbobbe
force-pushed
the
pr-edit-message-6
branch
from
8e330b2 to
2739149
Compare
|
Thanks! Revision pushed. |
Here's another small batch of miscellaneous prep work for the edit-message feature, #126.