Polish required in Filelink account setup dialog

RESOLVED FIXED in Thunderbird 14.0

Status

Thunderbird
Preferences
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
Thunderbird 14.0
x86
All
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird13 fixed)

Details

Attachments

(5 attachments, 26 obsolete attachments)

33.43 KB, image/png
Details
107.88 KB, image/png
Details
125.19 KB, image/png
Details
52.92 KB, image/png
Details
25.86 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 698925
Created attachment 609371 [details] [diff] [review]
WIP Patch v1

Fixed on Windows (screenshot forthcoming).  OSX and Ubuntu are next.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Created attachment 609376 [details] [diff] [review]
WIP Patch v2

Whoops - forgot about the Classic theme for Windows.  This should address it now.
Attachment #609371 - Attachment is obsolete: true
Created attachment 609380 [details] [diff] [review]
WIP Patch v3

I botched the generation of WIP Patch 2.  Recreating.
Attachment #609376 - Attachment is obsolete: true
Created attachment 609398 [details]
"Username" padding in Ubuntu
Created attachment 609399 [details] [diff] [review]
WIP Patch v4
Attachment #609380 - Attachment is obsolete: true
Created attachment 609421 [details] [diff] [review]
Patch v1

Ok, fixed for OSX too.  Screenshot coming next.
Attachment #609399 - Attachment is obsolete: true
Created attachment 609423 [details]
OSX
Created attachment 609430 [details]
Windows Aero
Created attachment 609431 [details]
Windows Classic
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
Attachment #609421 - Flags: ui-review?(bwinton)
Attachment #609421 - Flags: review?(nisses.mail)
The "need an account" label goes all the way to the left when I run this patch.
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
Attachment #609421 - Flags: review?(nisses.mail) → review-
Created attachment 609737 [details] [diff] [review]
Patch v2

Andreas:

Good catch!  Thanks. :)

-Mike
Attachment #609421 - Attachment is obsolete: true
Attachment #609421 - Flags: ui-review?(bwinton)
Attachment #609737 - Flags: ui-review?(bwinton)
Attachment #609737 - Flags: review?(nisses.mail)
Created attachment 610143 [details]
Corrected OSX (#providerForm #username width: 252px;)

Good eye, Blake. With the width set to 252px, it looks like this.
Attachment #609423 - Attachment is obsolete: true
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.
Attachment #609737 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 609737 [details] [diff] [review]
Patch v2

Cancelling review request, since all of this is likely to change with the new layout.
Attachment #609737 - Flags: review?(nisses.mail)
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.
Attachment #609398 - Attachment is obsolete: true
Attachment #609430 - Attachment is obsolete: true
Attachment #609431 - Attachment is obsolete: true
Attachment #609737 - Attachment is obsolete: true
Attachment #610143 - Attachment is obsolete: true
Summary: Polish required in YouSendIt setup dialog → Polish required in Filelink account setup dialog
Created attachment 610258 [details] [diff] [review]
WIP Patch v1

Looking pretty good on Ubuntu - going to do Windows and OSX next, and then screenshots.
Created attachment 610271 [details]
WIP Patch v2

Windows is taken care of now, moving on to OSX.
Attachment #610258 - Attachment is obsolete: true
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...
Attachment #610271 - Attachment is obsolete: true
Created attachment 610292 [details] [diff] [review]
WIP Patch v4

Getting closer to something I'm happy with.
Attachment #610280 - Attachment is obsolete: true
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.
Attachment #610292 - Attachment is obsolete: true
Created attachment 610639 [details] [diff] [review]
WIP Patch v6
Attachment #610632 - Attachment is obsolete: true
Created attachment 610648 [details] [diff] [review]
WIP Patch v7
Attachment #610639 - Attachment is obsolete: true
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
Attachment #610648 - Attachment is obsolete: true
Attachment #610651 - Flags: ui-review?(bwinton)
Attachment #610651 - Flags: review?(bwinton)
Blocks: 737349
Blocks: 738699
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.
Attachment #610651 - Flags: ui-review?(bwinton)
Attachment #610651 - Flags: ui-review-
Attachment #610651 - Flags: review?(bwinton)
Attachment #610651 - Flags: review+
Blocks: 739631
Created attachment 614059 [details]
Dialog in Windows Aero theme
Created attachment 614072 [details]
Dialog in Windows XP
Created attachment 614080 [details] [diff] [review]
Fixed window size patch Dropbox and YouSendIt

Stashing work here for machine transfer
Created attachment 614089 [details] [diff] [review]
Patch v2
Attachment #610651 - Attachment is obsolete: true
Attachment #614080 - Attachment is obsolete: true
Created attachment 614090 [details]
Dialog in OSX

Starting a new batch of screenshots with the latest changes.
Attachment #614059 - Attachment is obsolete: true
Attachment #614072 - Attachment is obsolete: true
Created attachment 614097 [details]
Dialog in Windows 7
Created attachment 614098 [details]
Dialog in Windows XP
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.
Attachment #614089 - Attachment is obsolete: true
Attachment #614123 - Flags: ui-review?(bwinton)
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.
Attachment #614123 - Flags: ui-review?(bwinton) → ui-review+

Comment 36

5 years ago
Could you please make sure #737349 is addressed ? Judging from the provided screen shots, this is not the case yet.
(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
Created attachment 614382 [details] [diff] [review]
Patch v4 WIP 1
Attachment #614123 - Attachment is obsolete: true
Created attachment 614394 [details] [diff] [review]
Patch v4 WIP 2
Attachment #614382 - Attachment is obsolete: true
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!
Attachment #614394 - Attachment is obsolete: true
Attachment #614405 - Attachment description: Patch v4 → Patch v4 (r+ and ui-r+ from bwinton)
Attachment #614405 - Flags: approval-comm-aurora?
Committed to comm-central as http://hg.mozilla.org/comm-central/rev/c79faac0f542
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0

Updated

5 years ago
Attachment #614405 - Flags: approval-comm-aurora? → approval-comm-aurora+
Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/bc7bd2dee00d
status-thunderbird13: --- → fixed
You need to log in before you can comment on or make changes to this bug.