Last Comment Bug 739279 - Polish required in Filelink account setup dialog
: Polish required in Filelink account setup dialog
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
Mentors:
Depends on:
Blocks: BigFiles 737349 738699 739631
  Show dependency treegraph
 
Reported: 2012-03-26 09:47 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-04-12 13:48 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
WIP Patch v1 (736 bytes, patch)
2012-03-26 10:28 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
WIP Patch v2 (1.18 KB, patch)
2012-03-26 10:44 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
WIP Patch v3 (1.32 KB, patch)
2012-03-26 10:53 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
"Username" padding in Ubuntu (21.24 KB, image/png)
2012-03-26 11:32 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
WIP Patch v4 (1.46 KB, patch)
2012-03-26 11:35 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Patch v1 (2.77 KB, patch)
2012-03-26 12:22 PDT, Mike Conley (:mconley) - (needinfo me!)
bugs: review-
Details | Diff | Review
OSX (22.41 KB, image/png)
2012-03-26 12:23 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows Aero (23.80 KB, image/png)
2012-03-26 12:27 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Windows Classic (10.98 KB, image/png)
2012-03-26 12:27 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Patch v2 (2.72 KB, patch)
2012-03-27 08:50 PDT, Mike Conley (:mconley) - (needinfo me!)
bwinton: ui‑review+
Details | Diff | Review
Corrected OSX (#providerForm #username width: 252px;) (22.63 KB, image/png)
2012-03-28 08:05 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
New layout wireframe (33.43 KB, image/png)
2012-03-28 09:29 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
WIP Patch v1 (9.25 KB, patch)
2012-03-28 12:54 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
WIP Patch v2 (11.70 KB, text/plain)
2012-03-28 13:21 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
WIP Patch v3 (14.99 KB, patch)
2012-03-28 13:36 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
WIP Patch v4 (18.86 KB, patch)
2012-03-28 14:09 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
WIP Patch v5 (18.20 KB, patch)
2012-03-29 11:58 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
WIP Patch v6 (18.47 KB, patch)
2012-03-29 12:15 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
WIP Patch v7 (21.60 KB, patch)
2012-03-29 12:34 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Patch v1 (22.07 KB, patch)
2012-03-29 12:48 PDT, Mike Conley (:mconley) - (needinfo me!)
bwinton: review+
bwinton: ui‑review-
Details | Diff | Review
Dialog in Windows Aero theme (163.61 KB, image/png)
2012-04-11 10:50 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Dialog in Windows XP (54.53 KB, image/png)
2012-04-11 11:16 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Fixed window size patch Dropbox and YouSendIt (1.98 KB, patch)
2012-04-11 11:33 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Patch v2 (22.32 KB, patch)
2012-04-11 11:52 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Dialog in OSX (107.88 KB, image/png)
2012-04-11 11:57 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Dialog in Windows 7 (125.19 KB, image/png)
2012-04-11 12:08 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Dialog in Windows XP (52.92 KB, image/png)
2012-04-11 12:08 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details
Patch v3 (carrying over r+ from bwinton) (23.55 KB, patch)
2012-04-11 12:47 PDT, Mike Conley (:mconley) - (needinfo me!)
bwinton: ui‑review+
Details | Diff | Review
Patch v4 WIP 1 (25.75 KB, patch)
2012-04-12 08:32 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Patch v4 WIP 2 (25.96 KB, patch)
2012-04-12 08:54 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Review
Patch v4 (r+ and ui-r+ from bwinton) (25.86 KB, patch)
2012-04-12 09:29 PDT, Mike Conley (:mconley) - (needinfo me!)
mozilla: approval‑comm‑aurora+
Details | Diff | Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-03-26 09:47:33 PDT
The YouSendIt setup dialog has a few tiny issues.

Across qute/pinstripe/gnomestrip, the "Username:" label needs some space to the right before the text input.

And on Windows, the username text input needs to terminate at the same point that the dropdown menu terminates.
Comment 1 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 10:28:55 PDT
Created attachment 609371 [details] [diff] [review]
WIP Patch v1

Fixed on Windows (screenshot forthcoming).  OSX and Ubuntu are next.
Comment 2 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 10:44:32 PDT
Created attachment 609376 [details] [diff] [review]
WIP Patch v2

Whoops - forgot about the Classic theme for Windows.  This should address it now.
Comment 3 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 10:53:54 PDT
Created attachment 609380 [details] [diff] [review]
WIP Patch v3

I botched the generation of WIP Patch 2.  Recreating.
Comment 4 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 11:32:26 PDT
Created attachment 609398 [details]
"Username" padding in Ubuntu
Comment 5 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 11:35:05 PDT
Created attachment 609399 [details] [diff] [review]
WIP Patch v4
Comment 6 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 12:22:27 PDT
Created attachment 609421 [details] [diff] [review]
Patch v1

Ok, fixed for OSX too.  Screenshot coming next.
Comment 7 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 12:23:04 PDT
Created attachment 609423 [details]
OSX
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 12:27:40 PDT
Created attachment 609430 [details]
Windows Aero
Comment 9 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 12:27:57 PDT
Created attachment 609431 [details]
Windows Classic
Comment 10 Mike Conley (:mconley) - (needinfo me!) 2012-03-26 12:28:43 PDT
Comment on attachment 609421 [details] [diff] [review]
Patch v1

Andreas:

Hey - how's my driving here?  Did I do the right thing wrt differentiating between Classic / Aero?

-Mike
Comment 11 Andreas Nilsson (:andreasn) 2012-03-27 05:41:14 PDT
The "need an account" label goes all the way to the left when I run this patch.
Comment 12 Andreas Nilsson (:andreasn) 2012-03-27 06:13:50 PDT
Comment on attachment 609421 [details] [diff] [review]
Patch v1

+@media (-moz-windows-compositor) {
+  #providerForm #username {
+    width: 245px;
+  }
+}

I think you want "@media (-moz-windows-default-theme)" here instead so it works on the Windows Basic theme as well. -moz-windows-compositor only works on Aero Glass.

https://developer.mozilla.org/en/CSS/Media_queries#-moz-windows-compositor
Comment 13 Mike Conley (:mconley) - (needinfo me!) 2012-03-27 08:50:39 PDT
Created attachment 609737 [details] [diff] [review]
Patch v2

Andreas:

Good catch!  Thanks. :)

-Mike
Comment 14 Mike Conley (:mconley) - (needinfo me!) 2012-03-28 08:05:52 PDT
Created attachment 610143 [details]
Corrected OSX (#providerForm #username width: 252px;)

Good eye, Blake. With the width set to 252px, it looks like this.
Comment 15 Blake Winton (:bwinton) (:☕️) 2012-03-28 08:15:51 PDT
Comment on attachment 609737 [details] [diff] [review]
Patch v2

So, I think I want the "Username" label to line up with the "Set up Filelink" label (and the text below that).

And the end of the text field on Mac should line up with the end of the dropdown.  (Which you've fixed in the Corrected version.)

The field in Ubuntu is two pixels too long.

Windows looks fine.

ui-r=me with those things fixed.  ;)

Thanks,
Blake.
Comment 16 Mike Conley (:mconley) - (needinfo me!) 2012-03-28 09:28:36 PDT
Comment on attachment 609737 [details] [diff] [review]
Patch v2

Cancelling review request, since all of this is likely to change with the new layout.
Comment 17 Mike Conley (:mconley) - (needinfo me!) 2012-03-28 09:29:40 PDT
Created attachment 610169 [details]
New layout wireframe

After speaking with andreasn and bwinton over IRC, we're going to change the layout of the dialog somewhat, to make ourselves more resilient to crazy-long strings.
Comment 18 Mike Conley (:mconley) - (needinfo me!) 2012-03-28 12:54:26 PDT
Created attachment 610258 [details] [diff] [review]
WIP Patch v1

Looking pretty good on Ubuntu - going to do Windows and OSX next, and then screenshots.
Comment 19 Mike Conley (:mconley) - (needinfo me!) 2012-03-28 13:21:18 PDT
Created attachment 610271 [details]
WIP Patch v2

Windows is taken care of now, moving on to OSX.
Comment 20 Mike Conley (:mconley) - (needinfo me!) 2012-03-28 13:36:17 PDT
Created attachment 610280 [details] [diff] [review]
WIP Patch v3

Ok, OSX is done.  Just one small behavioural bug to polish, and then I think this'll be ready for some UI review...
Comment 21 Mike Conley (:mconley) - (needinfo me!) 2012-03-28 14:09:01 PDT
Created attachment 610292 [details] [diff] [review]
WIP Patch v4

Getting closer to something I'm happy with.
Comment 22 Mike Conley (:mconley) - (needinfo me!) 2012-03-29 11:58:11 PDT
Created attachment 610632 [details] [diff] [review]
WIP Patch v5

Fixes up some fiddly window resize issues, and also makes the first form field focus in the YouSendIt settings form.
Comment 23 Mike Conley (:mconley) - (needinfo me!) 2012-03-29 12:15:14 PDT
Created attachment 610639 [details] [diff] [review]
WIP Patch v6
Comment 24 Mike Conley (:mconley) - (needinfo me!) 2012-03-29 12:34:18 PDT
Created attachment 610648 [details] [diff] [review]
WIP Patch v7
Comment 25 Mike Conley (:mconley) - (needinfo me!) 2012-03-29 12:48:59 PDT
Created attachment 610651 [details] [diff] [review]
Patch v1

Sorry to toss another one on the review queue, bwinton.  I'll grab a few more off your stack, and we'll be even.  :D
Comment 26 Blake Winton (:bwinton) (:☕️) 2012-04-10 14:52:07 PDT
Comment on attachment 610651 [details] [diff] [review]
Patch v1

So, comments on the OS X dialog, since that's the only one I've seen so far…  ;)

The padding on the left and top is different than the padding on the bottom and right, as shown in http://dl.dropbox.com/u/2301433/Screenshots/AccountSpacing.png

I'm not a huge fan of the way the dialog changes size when I select Dropbox or YouSendIt.  Can we make it big enough for both?  (Uh, not that we shouldn't resize the frame when we have to, just that I'ld rather not for the two cases we ship with in the default language.  ;)

Saying "This feature allows you to…" sounds off.  We don't really talk about our features by calling them "this feature" in Thunderbird anywhere else.  I would prefer something like "You can send large attachments using one of several online storage services.  Please either connect to an existing account, or sign up for a new account."

So, based on those, ui-r-.  (But they should hopefully be easy to fix.  Also, if you want to post screenshots of Windows Aero, Windows Classic, and Linux, I'll let you know what I want fixed in them, too.)

>+++ b/mail/components/cloudfile/content/addAccountDialog.js	Thu Mar 29 15:47:44 2012 -0400
>@@ -1,14 +1,14 @@
>-const kFormId = "providerForm";
>+const kFormId = "provider-form";

Should we also change
mail/themes/qute/mail/preferences/preferences-aero.css
64:  #providerForm #username {
?

>@@ -72,17 +71,37 @@ let addAccountDialog = {
>       addAccountDialog.onInput();
>+      addAccountDialog.resizeIFrame();
[snip…]
>+    addAccountDialog.resizeIFrame();

Do you really need to call this twice?

>+++ b/mail/components/cloudfile/content/emptySettings.xhtml	Thu Mar 29 15:47:44 2012 -0400
>@@ -0,0 +1,13 @@
>+<?xml version="1.0" encoding="UTF-8"?>
>+<!DOCTYPE html
>+PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
>+"DTD/xhtml1-strict.dtd">
>+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">

I think we need the mpl2 boilerplate comment somewhere near here.

But aside from that, I'm pretty happy with the code, so r=me with those things fixed.

Thanks,
Blake.
Comment 27 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 10:50:00 PDT
Created attachment 614059 [details]
Dialog in Windows Aero theme
Comment 28 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 11:16:15 PDT
Created attachment 614072 [details]
Dialog in Windows XP
Comment 29 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 11:33:38 PDT
Created attachment 614080 [details] [diff] [review]
Fixed window size patch Dropbox and YouSendIt

Stashing work here for machine transfer
Comment 30 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 11:52:27 PDT
Created attachment 614089 [details] [diff] [review]
Patch v2
Comment 31 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 11:57:52 PDT
Created attachment 614090 [details]
Dialog in OSX

Starting a new batch of screenshots with the latest changes.
Comment 32 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 12:08:38 PDT
Created attachment 614097 [details]
Dialog in Windows 7
Comment 33 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 12:08:58 PDT
Created attachment 614098 [details]
Dialog in Windows XP
Comment 34 Mike Conley (:mconley) - (needinfo me!) 2012-04-11 12:47:07 PDT
Created attachment 614123 [details] [diff] [review]
Patch v3 (carrying over r+ from bwinton)

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #26)
> Comment on attachment 610651 [details] [diff] [review]
> Patch v1
> 
> So, comments on the OS X dialog, since that's the only one I've seen so far…
> ;)
> 
> The padding on the left and top is different than the padding on the bottom
> and right, as shown in
> http://dl.dropbox.com/u/2301433/Screenshots/AccountSpacing.png

Good catch!  Turns out that dialogs in OSX and Ubuntu have some implicit margins on the right size of their buttons.  Fixed.

> 
> I'm not a huge fan of the way the dialog changes size when I select Dropbox
> or YouSendIt.  Can we make it big enough for both?  (Uh, not that we
> shouldn't resize the frame when we have to, just that I'ld rather not for
> the two cases we ship with in the default language.  ;)

Fixed.  In the event that we *do* overflow with some future provider, the window will resize itself.

> 
> Saying "This feature allows you to…" sounds off.  We don't really talk about
> our features by calling them "this feature" in Thunderbird anywhere else.  I
> would prefer something like "You can send large attachments using one of
> several online storage services.  Please either connect to an existing
> account, or sign up for a new account."

I'll prepare a separate patch for comm-central with the new string once this gets approval.

> 
> So, based on those, ui-r-.  (But they should hopefully be easy to fix. 
> Also, if you want to post screenshots of Windows Aero, Windows Classic, and
> Linux, I'll let you know what I want fixed in them, too.)

Posted.

> 
> >+++ b/mail/components/cloudfile/content/addAccountDialog.js	Thu Mar 29 15:47:44 2012 -0400
> >@@ -1,14 +1,14 @@
> >-const kFormId = "providerForm";
> >+const kFormId = "provider-form";
> 
> Should we also change
> mail/themes/qute/mail/preferences/preferences-aero.css
> 64:  #providerForm #username {
> ?

Good catch!  We don't even need that anymore.  Excised.

> 
> >@@ -72,17 +71,37 @@ let addAccountDialog = {
> >       addAccountDialog.onInput();
> >+      addAccountDialog.resizeIFrame();
> [snip…]
> >+    addAccountDialog.resizeIFrame();
> 
> Do you really need to call this twice?

Not anymore, now instead of calling it on IFrame content load, we call it on overflow.

> 
> >+++ b/mail/components/cloudfile/content/emptySettings.xhtml	Thu Mar 29 15:47:44 2012 -0400
> >@@ -0,0 +1,13 @@
> >+<?xml version="1.0" encoding="UTF-8"?>
> >+<!DOCTYPE html
> >+PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
> >+"DTD/xhtml1-strict.dtd">
> >+<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
> 
> I think we need the mpl2 boilerplate comment somewhere near here.

Good call.  Fixed.

> 
> But aside from that, I'm pretty happy with the code, so r=me with those
> things fixed.
> 
> Thanks,
> Blake.

Sweet, thanks.
Comment 35 Blake Winton (:bwinton) (:☕️) 2012-04-11 14:42:21 PDT
Comment on attachment 614123 [details] [diff] [review]
Patch v3 (carrying over r+ from bwinton)

Drive-by UX complaint: When I remove an account the top line says "[JavaScript Application]" in bold.  Do not want.  ;)

If there is only one provider selection people can make, can we just choose that for them, instead of making them drop it down themselves?

(OSX) So, now the left-right padding is 15px, but the top-bottom padding is 18px, which is closer, but I think using 18px everywhere would be even better.  ;)

On Windows Classic, the Username and Need an Account text is aligned with the edge of the text fields, but on Windows 7 and OS X, they're indented by a pixel.  I think I could live with either, but we should be consistent.

Other than that, I like it.  ui-r=me with those fixed.
Comment 36 Jb Piacentino 2012-04-11 23:40:15 PDT
Could you please make sure #737349 is addressed ? Judging from the provided screen shots, this is not the case yet.
Comment 37 Mike Conley (:mconley) - (needinfo me!) 2012-04-12 08:11:39 PDT
(In reply to Jb Piacentino from comment #36)
> Could you please make sure #737349 is addressed ? Judging from the provided
> screen shots, this is not the case yet.

Jb:

That bug will be addressed once this one lands, since it depends on it.

-Mike
Comment 38 Mike Conley (:mconley) - (needinfo me!) 2012-04-12 08:32:20 PDT
Created attachment 614382 [details] [diff] [review]
Patch v4 WIP 1
Comment 39 Mike Conley (:mconley) - (needinfo me!) 2012-04-12 08:54:50 PDT
Created attachment 614394 [details] [diff] [review]
Patch v4 WIP 2
Comment 40 Mike Conley (:mconley) - (needinfo me!) 2012-04-12 09:29:02 PDT
Created attachment 614405 [details] [diff] [review]
Patch v4 (r+ and ui-r+ from bwinton)

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #35)
> Comment on attachment 614123 [details] [diff] [review]
> Patch v3 (carrying over r+ from bwinton)
> 
> Drive-by UX complaint: When I remove an account the top line says
> "[JavaScript Application]" in bold.  Do not want.  ;)
> 

Good catch!  We were accidentally using the window-level "confirm" function, as opposed to using nsIPromptService.  I switched that, and now the header defaults to "Confirm", which I guess it the default.

> If there is only one provider selection people can make, can we just choose
> that for them, instead of making them drop it down themselves?
> 

Sure thing, done.

> (OSX) So, now the left-right padding is 15px, but the top-bottom padding is
> 18px, which is closer, but I think using 18px everywhere would be even
> better.  ;)
> 

Fixed!

> On Windows Classic, the Username and Need an Account text is aligned with
> the edge of the text fields, but on Windows 7 and OS X, they're indented by
> a pixel.  I think I could live with either, but we should be consistent.
> 

Ah, sub-pixel rendering.  So, to compensate, I've moved the label in Windows Classic in by a pixel.

> Other than that, I like it.  ui-r=me with those fixed.

Thanks!
Comment 41 Mike Conley (:mconley) - (needinfo me!) 2012-04-12 09:35:37 PDT
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/c79faac0f542
Comment 42 Mike Conley (:mconley) - (needinfo me!) 2012-04-12 13:48:47 PDT
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/bc7bd2dee00d

Note You need to log in before you can comment on or make changes to this bug.