Skip to content
This repository has been archived by the owner on Dec 13, 2020. It is now read-only.

Wait with closing the modal until all PATCH requests are finished #1500

Merged
merged 7 commits into from
Jan 19, 2018

Conversation

pablosichert
Copy link
Contributor

#1361

@teosarca: What should happen if one of the requests failed? Currently I just prevent the modal from closing and do nothing after.

@teosarca
Copy link
Member

teosarca commented Jan 13, 2018

What should happen if one of the requests failed? Currently I just prevent the modal from closing and do nothing after.

@pablosichert hmm... but that means, in case any of PATCH requests fails, user will never be able to close the window. right?
If that's the case, i think it's not OK. We shall be a bit resilient. Any idea how to have some resilience mechanism quick and simple?

@pablosichert
Copy link
Contributor Author

in case any of PATCH requests fails, user will never be able to close the window. right?

No, not quite. Just his current attempt will be aborted (by leaving the modal open). If he clicks the done button right after again, he will be able to close it.

@pablosichert
Copy link
Contributor Author

pablosichert commented Jan 13, 2018

My question is rather: Is it too subtle for the user to notice that something went wrong if we just leave the modal open? Should we give a more explicit hint?

@teosarca
Copy link
Member

My question is rather: Is it to subtle for the user to notice that something went wrong if we just leave the modal open? Should we give a more explicit hint?

Not sure I am picturing it correctly, but in case the user presses Done and the window is not closing because of some endpoint call which failed, IMHO, we shall display that error because else user will not understand what happened. wdyt?

@pablosichert
Copy link
Contributor Author

we shall display that error because else user will not understand what happened

👍

I think the default error notification at the top right will appear when one of the requests errors, but can't confirm currently.

@pablosichert
Copy link
Contributor Author

I think the default error notification at the top right will appear when one of the requests errors, but can't confirm currently.

Just checked - It just fails silently. Will add the error notification

@pablosichert
Copy link
Contributor Author

Checked again - I didn't get an error notification because of the way I mocked out a failed request.

There's no special case here, so we should get the error notification when a PATCH request goes wrong.

@teosarca I can add an extra error notification informing the user why the modal could not close, wdyt?

@metas-mk
Copy link
Member

metas-mk commented Jan 19, 2018

Checked locally. The issue solved. Integrating.

@metas-mk metas-mk merged commit 2fed7e3 into master Jan 19, 2018
@metas-mk metas-mk deleted the dev-1361 branch January 19, 2018 10:10
@teosarca
Copy link
Member

@pablosichert let's see how it turns in testing... cannot make up my mind

@pablosichert
Copy link
Contributor Author

cannot make up my mind

Regarding what? 😄 If this thing is resilient?

@teosarca
Copy link
Member

about this:

I can add an extra error notification informing the user why the modal could not close, wdyt?

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

Successfully merging this pull request may close these issues.

3 participants