msglist: Tapping a message in Starred or Mentions opens anchored msglist #1657
Conversation
Adapted from the similar test for tapping the topic: > 'navigates to TopicNarrow on tapping topic in ChannelNarrow'
chrisbobbe
added
the
integration review
|
(fixing CI; just an unused import.) |
chrisbobbe
force-pushed
the
pr-tap-msg-open-conversation
branch
from
75caabe to
f35dc2a
Compare
lib/widgets/message_list.dart
Outdated
| MessageListMessageItem() => MessageWithPossibleSender( | ||
| item: item, | ||
| tapOpensConversation: tapOpensConversation, | ||
| ), |
There was a problem hiding this comment.
nit: item is the payload, tapOpensConversation is metadata, so keep the payload last in the widget constructor's parameter list
lib/widgets/message_list.dart
Outdated
| /// Whether tapping a message should open its conversation. | ||
| bool get _tapMessageOpensConversation => switch (widget.narrow) { | ||
| CombinedFeedNarrow() | ||
| || ChannelNarrow() | ||
| || TopicNarrow() | ||
| || DmNarrow() => false, |
There was a problem hiding this comment.
nit: I think it's probably cleaner to have this logic live next to where it gets used, on MessageWithPossibleSender; so instead of passing down the bool this returns, we'd pass down the Narrow itself. Just like we do with RecipientHeader now.
| check(lastPushedRoute).isNull(); | ||
| } | ||
|
|
||
| // TODO test tapping whitespace in message |
There was a problem hiding this comment.
I guess this TODO test is the one that would fail if the HitTestBehavior.translucent were changed to deferToChild.
Leaving a TODO seems fine.
There was a problem hiding this comment.
Yeah; I timed out trying to simulate tapping the back button to clear the state from the previous part of the test (find.backButton() wasn't working)
test/widgets/message_list_test.dart
Outdated
| doTest(CombinedFeedNarrow(), false, mkMessage: () => eg.streamMessage()); | ||
| doTest(ChannelNarrow(subscription.streamId), false, | ||
| mkMessage: () => eg.streamMessage(stream: subscription)); | ||
| doTest(TopicNarrow(subscription.streamId, eg.t(topic)), false, |
There was a problem hiding this comment.
nit: can I think clarify by reformatting slightly:
| doTest(CombinedFeedNarrow(), false, mkMessage: () => eg.streamMessage()); | |
| doTest(ChannelNarrow(subscription.streamId), false, | |
| mkMessage: () => eg.streamMessage(stream: subscription)); | |
| doTest(TopicNarrow(subscription.streamId, eg.t(topic)), false, | |
| doTest(expected: false, CombinedFeedNarrow(), | |
| mkMessage: () => eg.streamMessage()); | |
| doTest(expected: false, ChannelNarrow(subscription.streamId), | |
| mkMessage: () => eg.streamMessage(stream: subscription)); | |
| doTest(expected: false, TopicNarrow(subscription.streamId, eg.t(topic)), |
- name the boolean parameter, so its meaning is clear;
- put the short, near-constant-width argument first, so corresponding arguments are aligned;
- put this
mkMessageon the next line, to match the others.
chrisbobbe
force-pushed
the
pr-tap-msg-open-conversation
branch
from
f35dc2a to
a922f56
Compare
|
Thanks for the review! Revision pushed. |
|
Thanks! Looks good; merging. |
Fixes #1621.