-
Notifications
You must be signed in to change notification settings - Fork 308
local echo (7/7): Support simplified version of local echo #1453
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
Conversation
988c615
to
f297a65
Compare
aa81e2f
to
28d545f
Compare
This is where the progress bar for outbox messages will go, so this is for consistency with that. Discussion: zulip#1453 (comment) [chris: fixed to maintain 4px bottom padding in the common case where the progress bar is absent] Co-authored-by: Chris Bobbe <[email protected]>
And I've sent revised versions of the next chunk, in #1535. |
This is where the progress bar for outbox messages will go, so this is for consistency with that. Discussion: zulip#1453 (comment) [chris: fixed to maintain 4px bottom padding in the common case where the progress bar is absent] Co-authored-by: Chris Bobbe <[email protected]>
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>
OK, and now rebased with a revision ready for review! This is stacked atop #1538, which is a separate PR because it's a substantial commit. |
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.
Thanks for picking this up! I've read through the commits after #1538:
72ffbd8 compose: Remove a redundant TypingNotifier.stoppedComposing call
3b9366e compose [nfc]: Remove obsoleted TODO(#720)s
d56e20f compose [nfc]: Expand a comment to include an edge case
419e17b msglist: Support retrieving failed outbox message content
except the tests of the last/main commit. All looks good except one small comment below. (And I should read the tests too.)
lib/widgets/message_list.dart
Outdated
Widget build(BuildContext context) { | ||
final message = item.message; | ||
final localMessageId = message.localMessageId; | ||
final state = item.message.state; |
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.
Maybe just say message.state
in the two places this is used. I feel like a bare name state
could be confusing in the context of a widget — sounds like the state of some stateful widget.
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>
Thanks for the review! Revision pushed, atop the current #1538. (edit: and again, resolving a small conflict) |
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>
Rebased on current |
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.
Thanks! Also just read through those tests; a handful of comments below.
final controller = this.controller; | ||
controller | ||
..content.value = TextEditingValue(text: outboxMessage.contentMarkdown) | ||
..contentFocusNode.requestFocus(); |
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 bit should ideally get a test.
In the interest of merging this for launch, let's just leave a TODO comment for it in the test code.
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 easy to test :) done.
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.
Cool, looks good :)
test/widgets/message_list_test.dart
Outdated
// Navigate back to the message list page without a compose box, | ||
// where the failed to send message should be visible. | ||
await tester.pageBack(); | ||
await tester.pump(); // handle tap | ||
await tester.pump(const Duration(milliseconds: 500)); // wait for navigation |
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.
Should this be adapted to not specify the duration, per that upstream PR?
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.
Because customer_testing pins the version of our repo, I guess it's not an immediate problem (even for that upstream PR) if we merge it this way. But if the upstream PR lands and this is already merged, it'd break our CI.
test/widgets/message_list_test.dart
Outdated
await checkTapNotRestoreMessage(tester); | ||
}); | ||
|
||
testWidgets('hidden -> failed, tap to restore message', (tester) async { |
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.
nit: perhaps put this above the "tapping does nothing if compose box is not offered" case — this is a lot simpler and serves as a sort of baseline for understanding the more complex cases
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>
Thanks for the review! Revision pushed. |
Thanks! Looks good; merging. … perhaps after #1568 lands, in order to pin that one down with certainty without another CI cycle. |
Issue zulip#720 is superseded by zulip#1441, in which we'll still clear the compose box when the send button is tapped. (We'll still preserve the composing progress in case the send fails, but we'll do so in an OutboxMessage instead of within the compose input in a disabled state.)
Issue zulip#720 is superseded by zulip#1441, and these don't apply... I guess with the exception of a note on how a generic "x" button could be laid out, so we leave that.
Different from the Figma design, the bottom padding below the progress bar is changed from 0.5px to 2px, as discussed here: zulip#1453 (comment) Fixes: zulip#1441 Co-authored-by: Chris Bobbe <[email protected]>
Fixes #1441.
Stacked atop #1472.
(This branch can be used to preview the full implementation)
screenshots