-
Notifications
You must be signed in to change notification settings - Fork 306
Await send request; handle failure #720
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
Comments
I've filed the first steps of this feature, which will already be useful in themselves, as a separate issue: |
Does this reflect the logic? |
That looks good, thanks! I think we'll need to allow a much longer timeout than 5 seconds — sometimes people are on slow network connections (or flaky ones such that the network needs to retry the packets), and sometimes their server is slow. Something like 30 seconds is probably long enough. Because that'll be a while and the user may want to give up and move on sooner, there's this item we had above:
|
The new design part of this is probably blocked by #915. I think we should move this to the "Launch" milestone as well. |
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
We've been using ComposeBoxController as an interface, implemented by _StreamComposeBoxState and _FixedDestinationComposeBoxState. This commit puts the relevant implementation in ComposeBoxController itself -- really in subclasses StreamComposeBoxController and FixedDestinationComposeBoxController -- and makes those _FooState classes instantiate it and store the instance in a field. Next, we'll move the field up from those _FooState classes onto ComposeBox's state to make the controller straightforwardly available there (i.e. available without going through GlobalKey logic). That will be helpful for zulip#720 when the controller grows a send-in-progress flag; ComposeBox will be a conveniently central place to build a progress indicator for that state.
This widget is already doing more than layout (sizing and positioning its inputs on the screen): it creates the attachment buttons and styles their touch feedback, and it defeats an unwanted border that its `topicInput` and `contentInput` params might otherwise create. I chose the name "body" to distinguish this from some other elements that will need to share the compose-box surface, for the UI part of issue zulip#720: an error banner and a linear progress indicator. Then also add "body" to the names _StreamComposeBox and _FixedDestinationComposeBox. Since the recent commits that simplified those, they're really just thin, stateless wrappers that configure a _ComposeBoxBody.
Now, _ComposeBoxContainer is available as a nice place to put things other than the "body" that (unlike the body) will want to consume the left and right insets. Specifically, for zulip#720, an error banner and an overlaid progress indicator. Next, we'll rename `_ComposeBoxContainer.child` to `body`. We'll also go ahead and add a param for the error banner, redesign the error banner that we currently have and are passing as `child` / `body`, and pass the banner as that new param instead.
This cherry-picks (with some adjustments) UI changes from Zixuan's current revision of PR zulip#924 (for issue zulip#720): compose: Support error banner redesign for replacing the compose box See the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0 The new design is meant to be flush with the top of the compose-box surface, so zulip#1089 (where the old banner's top margin disappeared, looking buggy) is resolved. The surface (but not content) of the redesigned banner seems like it should extend through any left/right device insets to those edges of the screen. By taking the banner as param separate from `body`, _ComposeBoxContainer lets the banner do that -- without dropping its responsibility for keeping the body (inputs, compose buttons, and send button) out of the insets. The body doesn't need to consume the insets itself and it's already complicated enough without needing its own SafeAreas. It also seems like the banner should pad the bottom inset when it's meant to replace the body, so we do that and make the expectation clear in the `errorBanner` dartdoc. Co-authored-by: Zixuan James Li <[email protected]> Fixes: zulip#1089
We've been using ComposeBoxController as an interface, implemented by _StreamComposeBoxState and _FixedDestinationComposeBoxState. This commit puts the relevant implementation in ComposeBoxController itself -- really in subclasses StreamComposeBoxController and FixedDestinationComposeBoxController -- and makes those _FooState classes instantiate it and store the instance in a field. Next, we'll move the field up from those _FooState classes onto ComposeBox's state to make the controller straightforwardly available there (i.e. available without going through GlobalKey logic). That will be helpful for zulip#720 when the controller grows a send-in-progress flag; ComposeBox will be a conveniently central place to build a progress indicator for that state.
This widget is already doing more than layout (sizing and positioning its inputs on the screen): it creates the attachment buttons and styles their touch feedback, and it defeats an unwanted border that its `topicInput` and `contentInput` params might otherwise create. I chose the name "body" to distinguish this from some other elements that will need to share the compose-box surface, for the UI part of issue zulip#720: an error banner and a linear progress indicator. Then also add "body" to the names _StreamComposeBox and _FixedDestinationComposeBox. Since the recent commits that simplified those, they're really just thin, stateless wrappers that configure a _ComposeBoxBody.
Now, _ComposeBoxContainer is available as a nice place to put things other than the "body" that (unlike the body) will want to consume the left and right insets. Specifically, for zulip#720, an error banner and an overlaid progress indicator. Next, we'll rename `_ComposeBoxContainer.child` to `body`. We'll also go ahead and add a param for the error banner, redesign the error banner that we currently have and are passing as `child` / `body`, and pass the banner as that new param instead.
This cherry-picks (with some adjustments) UI changes from Zixuan's current revision of PR zulip#924 (for issue zulip#720): compose: Support error banner redesign for replacing the compose box See the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0 The new design is meant to be flush with the top of the compose-box surface, so zulip#1089 (where the old banner's top margin disappeared, looking buggy) is resolved. The surface (but not content) of the redesigned banner seems like it should extend through any left/right device insets to those edges of the screen. By taking the banner as param separate from `body`, _ComposeBoxContainer lets the banner do that -- without dropping its responsibility for keeping the body (inputs, compose buttons, and send button) out of the insets. The body doesn't need to consume the insets itself and it's already complicated enough without needing its own SafeAreas. It also seems like the banner should pad the bottom inset when it's meant to replace the body, so we do that and make the expectation clear in the `errorBanner` dartdoc. Co-authored-by: Zixuan James Li <[email protected]> Fixes: zulip#1089
For zulip#720, we'd like to add fields to ComposeBoxController and use them in a bunch of widgets deeper in the compose box, like the various buttons, without having to add references to them in those widgets' constructors. With that in mind, pass the whole controller down, and the widgets can access the fields directly from it. As suggested by Greg: zulip#1090 (comment)
We've been using ComposeBoxController as an interface, implemented by _StreamComposeBoxState and _FixedDestinationComposeBoxState. This commit puts the relevant implementation in ComposeBoxController itself -- really in subclasses StreamComposeBoxController and FixedDestinationComposeBoxController -- and makes those _FooState classes instantiate it and store the instance in a field. Next, we'll move the field up from those _FooState classes onto ComposeBox's state to make the controller straightforwardly available there (i.e. available without going through GlobalKey logic). That will be helpful for zulip#720 when the controller grows a send-in-progress flag; ComposeBox will be a conveniently central place to build a progress indicator for that state.
This widget is already doing more than layout (sizing and positioning its inputs on the screen): it creates the attachment buttons and styles their touch feedback, and it defeats an unwanted border that its `topicInput` and `contentInput` params might otherwise create. I chose the name "body" to distinguish this from some other elements that will need to share the compose-box surface, for the UI part of issue zulip#720: an error banner and a linear progress indicator. Then also add "body" to the names _StreamComposeBox and _FixedDestinationComposeBox. Since the recent commits that simplified those, they're really just thin, stateless wrappers that configure a _ComposeBoxBody.
Now, _ComposeBoxContainer is available as a nice place to put things other than the "body" that (unlike the body) will want to consume the left and right insets. Specifically, for zulip#720, an error banner and an overlaid progress indicator. Next, we'll rename `_ComposeBoxContainer.child` to `body`. We'll also go ahead and add a param for the error banner, redesign the error banner that we currently have and are passing as `child` / `body`, and pass the banner as that new param instead.
This cherry-picks (with some adjustments) UI changes from Zixuan's current revision of PR zulip#924 (for issue zulip#720): compose: Support error banner redesign for replacing the compose box See the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-15770&node-type=frame&t=vOSEWlXAhBa5EsAv-0 The new design is meant to put the banner at the very top of the compose-box surface, with no margin. So zulip#1089, where the old banner's top margin disappeared, looking buggy, is resolved. The surface (but not content) of the redesigned banner seems like it should extend through any left/right device insets to those edges of the screen. By taking the banner as param separate from `body`, _ComposeBoxContainer lets the banner do that -- without dropping its responsibility for keeping the body (inputs, compose buttons, and send button) out of the insets. The body doesn't need to consume the insets itself and it's already complicated enough without needing its own SafeAreas. It also seems like the banner should pad the bottom inset when it's meant to replace the body, so we do that and make the expectation clear in the `errorBanner` dartdoc. Co-authored-by: Zixuan James Li <[email protected]> Fixes: zulip#1089
For zulip#720, we'd like to add fields to ComposeBoxController and use them in a bunch of widgets deeper in the compose box, like the various buttons, without having to add references to them in those widgets' constructors. With that in mind, pass the whole controller down, and the widgets can access the fields directly from it. As suggested by Greg: zulip#1090 (comment)
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Different from the Figma, we use a sharp border instead of a rounded one, because most of the time (for error messages of a single line) the 40x40 button is attached to the top/bottom/right border, and a rounded border would leave some blank space at the corners. We also align the button to the top when we soft wrap the error message; centering it leaves some awkward vertical padding. Apart from that, when the action button is not available (e.g. _ErrorBanner that replaces the compose box in narrows without post permission), the padding around the label is adjusted to center it. Fixes: zulip#720 Signed-off-by: Zixuan James Li <[email protected]>
Following up here on a recent discovery while working on this.
I think the next step will be at least pushing this to the next milestone, or figuring out a replacement design, unless we decide to go straight to the "outbox+local echo" solution. |
I think the next thing we'll do here is probably to attempt a version of local echo (#576). That means we'd bypass the specific design described above, for the reasons in the previous comment (and the discussion linked there). Leaving this issue thread open for now, though, as a way of tracking the user-facing need. |
We heard another report today of the unpleasant consequences of not yet having this:
|
Started a chat thread just now on how best to handle this situation: |
_ComposeBoxContainer also supports an error banner that doesn't replace the text inputs. We don't use that feature yet (the Figma has some examples where it'll be helpful: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-16992&m=dev and we used it in a draft toward zulip#720 that wasn't merged: zulip#720 (comment) ) but it seems helpful anyway to nail down this helper like this.
_ComposeBoxContainer also supports an error banner that doesn't replace the text inputs. We don't use that feature yet (the Figma has some examples where it'll be helpful: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=4031-16992&m=dev and we used it in a draft toward zulip#720 that wasn't merged: zulip#720 (comment) ) but it seems helpful anyway to nail down this helper like this.
I've filed #1441 to describe the new plan for handling this need. We'll track further work there. |
If you're using the app, then step into an elevator (or otherwise lose your network connection), and hit send on a message, the current experience is pretty bad: the message just vanishes, there's no error message, and you don't even get back the text you wrote so you can try again.
The UX we'd ultimately like to have here is local echo, with an "outbox" system that can retry the send when you're back on the network:
But those are complex to get right, and I think we won't get to them in the next few months. There's a much simpler thing we can do that would greatly mitigate this, so we should do the simpler thing first.
Specifically, when you hit the send button:
(To keep this simple to implement, we'll only be waiting for the send request to complete — not for the
message
event, which is what causes the message to appear full-fledged in the message list. This means that the message might appear in the list before or after the compose area goes back to ready. I'm hopeful that in practice this won't often be bothersome, and will be better than the status quo. If we find we need to wait for themessage
event in order for the interaction to feel right, then that gets a lot closer to a full version of #133, and we'll re-evaluate whether there's still a simple version that makes sense.)@terpimost will be preparing a more concrete design. Until that's ready, this issue isn't ready for implementation.
Thanks also to @terpimost for the feedback pointing out the need for this, in particular the elevator story :-)
The text was updated successfully, but these errors were encountered: