Skip to content
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

streams: Display error if stream update fails. #5289

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Fingel
Copy link
Contributor

@Fingel Fingel commented Mar 10, 2022

Displays a toast containing an error string if a call to
updateExistingStream returns a failed promise.

Fixes: #5286

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One comment.

Comment on lines 26 to 30
dispatch(
updateExistingStream(stream.stream_id, stream, { name, description, isPrivate }),
).catch(error => {
showToast(error.message);
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the Promise#catch method, use async/await and a try/catch.

There's a good thread somewhere explaining why, with examples from Anders too. I'll try to locate that and put an entry in the style guide.

@Fingel Fingel force-pushed the default-private-stream-error branch from 3c8c792 to 13c2da8 Compare March 10, 2022 21:18
Displays a toast containing an error string if a call to
updateExistingStream returns a failed promise.

Fixes: zulip#5286
@Fingel Fingel force-pushed the default-private-stream-error branch from 13c2da8 to 5e0387a Compare March 10, 2022 21:38
(name: string, description: string, isPrivate: boolean) => {
api.createStream(auth, name, description, [ownEmail], isPrivate);
async (name: string, description: string, isPrivate: boolean) => {
await api.createStream(auth, name, description, [ownEmail], isPrivate);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also preemptively add a try/catch and show a toast here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good thought -- let's handle this the same way.

@Fingel
Copy link
Contributor Author

Fingel commented Mar 10, 2022

@gnprice Does this look better? Admittedly I'm still kinda wrapping my head around what is going on here with the dispatch hook/async/ and promises.

@gnprice
Copy link
Member

gnprice commented Mar 11, 2022

It does! To note here what I said on our call this afternoon: while we're waiting for the request, let's show some UI feedback that things are happening: make the button disabled (both visually, and in the sense of not accepting a second press) and perhaps showing a spinner or something.

Ah, which we can do with the progress prop on ZulipButton. See the existing places we use that, like RealmInputScreen.


A smaller thing: in the case of failure, a modal alert is probably better than a toast. Then on dismissing the alert, stay on the same screen. That way if there's some way they can tweak the details and retry, they can do that without re-entering the other details.

See showErrorAlert for how to do that. (Hmm, it should probably get some jsdoc saying what it does.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No feedback shown when attempting to make a default stream private.
2 participants