518694 - [faceted search] clean layout and spinner progress for searches that take a while

Status: RESOLVED FIXED

(Thunderbird :: Search, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

right now if a gloda search takes a little while for search results the display is half filled and looks poorly arranged.  We need to lay out the results view statically while we are waiting for results to fill in and it would be good to have a "spinner" to indicate that the search is progressing.

Heads up to Andreas as this is probably very closely related to what we do in bug 515908

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

Just to follow up.  I think we could keep the side bar with the top elements disabled and replace the search results with a spinner and some text.  Likewise for the empty results I think we can do something similar to that.

I'm thinking about something along the lines of the data loading UI seen from the (exhibit?) code that Alex used for the icon inventory.
http://people.mozilla.com/~faaborg/files/granParadisoUI/icons/iconInventoryStatus.html

A center hovering box with some short (Loading...) text and a spinner.  I'm going to mark this as has l10n impact since it very likely will and we should be prepared for that.

Comment 2

[Andreas Nilsson (:andreasn)]

I like that. Perhaps the text could say "searching" instead of "working" in our case.

Comment 3

[Andreas Nilsson (:andreasn)]

Attachment: mockup

we can reuse the regular spinner here.

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: screnshot of loading search results

until bug 518694 lands the side bar is not going to fill the whole space like it should.  I have an outset border in there even though I don't like that. Without a border at all it seemed to blend into the white too much.

I'm going to look into hiding the "Filter By" top section completely until there are some results showing.  This would make the whole page look blue with white and the loading bar.

It would be good if I could get the Search: $query text to show up during this time but I might need to lean on asuth to make that happen.

Also that timeline icon keeps hanging around when it's not wanted.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: WIP for empty and loading search results

here's the work in progress patch that fixes this and bug 515908.  Just check pointing for now.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: updated patch

This patch takes care of the date toggle and hides the results message header when there are no results to show.  I also hid the empty ( ) for the top facets however we're still showing the checkboxes.

There is still some additional layout work I'd like to get done after this but I'm going to throw that over to Andreas once this patch with the JS and strings lands.

Thanks to wayne for some help with new empty search results string.

Maybe I can get a quick review from davida tomorrow and then over to someone else for a final.

Comment 7

[David Ascher (:davida)]

Comment on attachment 403436 [details] [diff] [review]
updated patch

Works ok, although i think that it takes too long for the loading page to show, which I suspect is due to the JS loading in parallel (on the main thread).  We might get a faster perceived speed with a judicious setTimeout at some point, but that can wait until after string freeze.

A couple of nits:

>diff --git a/mail/base/content/glodaFacetBindings.xml b/mail/base/content/glodaFacetBindings.xml
>--- a/mail/base/content/glodaFacetBindings.xml
>+++ b/mail/base/content/glodaFacetBindings.xml

>+        // set the count so CSS selectors can know what the results look like
>+        this.setAttribute("state", (totalCount <= 0)? "empty" : "some");
>+

I think the word 'state' and 'some are a bit vague.  I'd prefer "empty=true" or "empty=false" or "someresults=true", "someresults=false".  


>diff --git a/mail/base/content/glodaFacetView.css b/mail/base/content/glodaFacetView.css
>--- a/mail/base/content/glodaFacetView.css
>+++ b/mail/base/content/glodaFacetView.css
>@@ -196,17 +196,17 @@ h2 {
> 
> #date-toggle {
>   position: absolute;
>   right: 1em;
>   bottom: 0;
>   margin: 5px;
>   font-size: 90%;
>   text-align: right;
>-  display: inline-block;
>+  display: none;

add a comment like /* $('.date-toggle').show() is run if there are results */

> .results-message-header {
>-  display: table;
>+  display: none;

same.


>+}
>\ No newline at end of file

probably want a newline.

>diff --git a/mail/base/content/glodaFacetView.js b/mail/base/content/glodaFacetView.js
>--- a/mail/base/content/glodaFacetView.js
>+++ b/mail/base/content/glodaFacetView.js

>+    /* check for no messages at all */
>+    if (this._activeSet.length <= 0) {

isn't length == 0 good enough?


r=davida with those addressed.

Comment 8

[David Ascher (:davida)]

Hmm, one other thing i found -- with a too-narrow screen, there's injudicious word wrapping. Also, that magnifying glass is huuge.

Comment 9

[David Ascher (:davida)]

(that last comment is not blocking of the l10n-impacting checkin)

Comment 10

[David Ascher (:davida)]

(also -- i think one of this and the patch in 517449 will bitrot each other)

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #7)
> A couple of nits:
> 
> >diff --git a/mail/base/content/glodaFacetBindings.xml b/mail/base/content/glodaFacetBindings.xml
> >--- a/mail/base/content/glodaFacetBindings.xml
> >+++ b/mail/base/content/glodaFacetBindings.xml
> 
> >+        // set the count so CSS selectors can know what the results look like
> >+        this.setAttribute("state", (totalCount <= 0)? "empty" : "some");
> >+
> 
> I think the word 'state' and 'some are a bit vague.  I'd prefer "empty=true" or
> "empty=false" or "someresults=true", "someresults=false".  

That makes more sense to me but I was following a current pattern.  Your call.
http://mxr.mozilla.org/comm-central/source/mail/base/content/glodaFacetBindings.xml#762


Will fix up the other nits

Comment 12

[David Ascher (:davida)]

Good point about current pattern.  leave it.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: updated patch with nits fixed

Here's the updated patch for this with all the nits addressed.  Carrying r+ forward

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #10)
> (also -- i think one of this and the patch in 517449 will bitrot each other)

Will mark checkin-needed when bug 517449 lands and we're bitrot free.  For now we'll stay in a hovering pattern.

Comment 15

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: updated patch after bitrot

Asking for quick review again after de-bitrotting

Comment 16

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/c9c49ff76e22