If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Thunderbird 3.0rc1

Status

Thunderbird
Search
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: clarkbw, Assigned: clarkbw)

Tracking

({polish})

Trunk
Thunderbird 3.0rc1
polish
Bug Flags:
blocking-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has l10n impact])

Attachments

(3 attachments, 3 obsolete attachments)

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
Flags: blocking-thunderbird3+

Updated

8 years ago
Whiteboard: [no l10n impact]
(Assignee)

Comment 1

8 years ago
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.
Whiteboard: [no l10n impact] → [has l10n impact]
I like that. Perhaps the text could say "searching" instead of "working" in our case.
Created attachment 402810 [details]
mockup

we can reuse the regular spinner here.
(Assignee)

Updated

8 years ago
Depends on: 519051
(Assignee)

Updated

8 years ago
Whiteboard: [has l10n impact] → [has l10n impact][needs patch][waiting on 519051]
(Assignee)

Updated

8 years ago
Keywords: polish
Target Milestone: --- → Thunderbird 3.0rc1
(Assignee)

Comment 4

8 years ago
Created attachment 403354 [details]
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.
(Assignee)

Comment 5

8 years ago
Created attachment 403380 [details] [diff] [review]
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.
(Assignee)

Comment 6

8 years ago
Created attachment 403436 [details] [diff] [review]
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.
Attachment #403380 - Attachment is obsolete: true
Attachment #403436 - Flags: review?(david.ascher)
Whiteboard: [has l10n impact][needs patch][waiting on 519051] → [has l10n impact][needs review 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.
Attachment #403436 - Flags: review?(david.ascher) → review+
Hmm, one other thing i found -- with a too-narrow screen, there's injudicious word wrapping. Also, that magnifying glass is huuge.
(that last comment is not blocking of the l10n-impacting checkin)
(also -- i think one of this and the patch in 517449 will bitrot each other)
(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
Good point about current pattern.  leave it.
Created attachment 403521 [details] [diff] [review]
updated patch with nits fixed

Here's the updated patch for this with all the nits addressed.  Carrying r+ forward
Attachment #403436 - Attachment is obsolete: true
Attachment #403521 - Flags: review+
(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.
Created attachment 403535 [details] [diff] [review]
updated patch after bitrot

Asking for quick review again after de-bitrotting
Attachment #403521 - Attachment is obsolete: true
Attachment #403535 - Flags: review?(david.ascher)

Updated

8 years ago
Attachment #403535 - Flags: review?(david.ascher) → review+

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [has l10n impact][needs review davida] → [has l10n impact]
Checked in: http://hg.mozilla.org/comm-central/rev/c9c49ff76e22
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.