965354 - Don't display error dialog on aborted requests

Status: RESOLVED WONTFIX

(Tree Management Graveyard :: OrangeFactor, defect)

Description

[Mark Côté [:mcote]]

I believe that an error dialog is being displayed if an AJAX request is aborted, e.g. if you quickly switch to another page while the first one is still loading some data.  Since this is the result of an intentional user action, we shouldn't display errors when AJAX requests are aborted, only when there is a server error.

Comment 1

[Sarvesh( :s1991 )]

Attachment: Loader added on ajax loading

Comment 2

[Mark Côté [:mcote]]

Comment on attachment 8368636 [details] [diff] [review]
Loader added on ajax loading

Review of attachment 8368636 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like you forgot to include the image (do "hg add <path to image>" before doing the hg diff).

Comment 3

[Sarvesh( :s1991 )]

Attachment: Loader added on ajax loading

Comment 4

[Mark Côté [:mcote]]

Comment on attachment 8368688 [details] [diff] [review]
Loader added on ajax loading

Review of attachment 8368688 [details] [diff] [review]:
-----------------------------------------------------------------

So there are two problems here.  First, it didn't actually work for me at all, because the .ajaxStart() call occurred after the displayOverview() call, which starts the AJAX calls.  It seems that add the ajaxStart() handler after an AJAX call has been started doesn't work for the current or future AJAX calls either.  Did it really work for you?

Secondly, I would prefer a less obtrusive loader, maybe in the corner or somewhere else out of the way.  Since there are several AJAX calls in one page load, you shouldn't obscure the screen when there is useful data being shown, e.g. after the graph loads, while it's doing another AJAX call to get some data for a secondary table display.

Comment 5

[Sarvesh( :s1991 )]

Attachment: Loader added on ajax loading

Comment 6

[Sarvesh( :s1991 )]

Attachment: Loader added on ajax loading

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8370154 [details] [diff] [review]
Loader added on ajax loading

Review of attachment 8370154 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  I have a few comments, which can be fixed when I commit your patch, although I'd like an answer to the last one in particular.

::: html/scripts/woo.main.js
@@ +273,1 @@
>    displayOverview();

I think I would put in an extra blank line above, for clarity.

@@ +275,5 @@
>    // Initialize per-page controls (relies on global controls being loaded).
>    for (var i = 0; i < menus.length; i++) {
>      $(menus[i]).attr('href', PageLoader.buildUrl($(menus[i]).attr('id').slice(4), {}));
>    }
> +

Don't need the extra space here.

::: html/style/main.css
@@ +513,5 @@
> +.loader {
> +  position: absolute;
> +  left: 96%;
> +  top: 7%;
> +  position: fixed;

Why are there two "position" attributes?

Comment 8

[Ed Morley [:emorley]]

Wontfix in favour of OrangeFactor v2 which will consume Treeherder's API, and likely be written from scratch.