Need warning for moving cards via the message header if the cards exist in a list

RESOLVED FIXED in Thunderbird 5.0b1

Status

Thunderbird
Message Reader UI
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: standard8, Assigned: mconley)

Tracking

(Blocks: 1 bug, {dataloss})

Trunk
Thunderbird 5.0b1
dataloss
Dependency tree / graph

Firefox Tracking Flags

(blocking-thunderbird5.0 beta1+)

Details

(Whiteboard: [affects l10n])

Attachments

(7 attachments, 9 obsolete attachments)

35.52 KB, image/png
Details
48.54 KB, image/png
Details
39.45 KB, image/png
Details
21.09 KB, image/png
Details
18.52 KB, patch
bwinton
: review+
bwinton
: ui-review+
Details | Diff | Splinter Review
18.27 KB, image/png
Details
57.05 KB, image/png
Details
(Reporter)

Description

7 years ago
Follow-up to bug 456167.

We need a warning if cards exist in a mailing list and the user then attempts to move the card to a different address book.

Currently, that moving will cause the link to the mailing list to be lost without warning.
Flags: blocking-thunderbird-next?
(Reporter)

Updated

7 years ago
status-thunderbird3.2: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
(Reporter)

Updated

7 years ago
blocking-thunderbird5.0: --- → ?
Flags: blocking-thunderbird-next?
(Reporter)

Updated

6 years ago
blocking-thunderbird5.0: ? → alpha4+
status-thunderbird3.2: ? → ---
Whiteboard: [affects l10n]
(Reporter)

Comment 1

6 years ago
The full fix for this would be to make mailing lists capable of referencing contacts in other address books, but that's a bit complicated for 3.3.

Therefore the option suggested by Blake and myself is this:

- If the card is contained within a mailing list (within that address book), then disable the address book switching option on the Edit Contact pop-up within the message header pane.

- Add a small bit of text below the address book switching option that states something along the lines of 'this card is contained within mailing lists and cannot be moved'.
Whiteboard: [affects l10n] → [affects l10n][has proposed UI]
(Reporter)

Comment 2

6 years ago
Created attachment 529992 [details] [diff] [review]
Skeleton patch

This does the detection for whether or not a card is in a mail list, and disables the list box appropriately.

On Mac, at least, this will need some css so that the list looks disabled.

There's still a string to be added in as well.
(Assignee)

Updated

6 years ago
Assignee: nobody → mconley
(Assignee)

Comment 3

6 years ago
Created attachment 530091 [details] [diff] [review]
Patch v1

Here's my first run at a solution.

I'm open to suggestions on the wording of the warning.  I can imagine some languages (German comes to mind) where that sentence could get pretty long.

I've also updated the CSS to make the e-mail address readable, because it was mostly invisible before.  See the attached screenshots to see what I mean.  I couldn't find a bug for that one, so I thought I'd take care of that here - hope that's OK.
Attachment #529992 - Attachment is obsolete: true
Attachment #530091 - Flags: review?(mbanner)
(Assignee)

Comment 4

6 years ago
Created attachment 530092 [details]
Before patch - notice that the e-mail address is mostly invisible.
(Assignee)

Comment 5

6 years ago
Created attachment 530093 [details]
After patch - the text is read-only, and more visible.
(Assignee)

Comment 6

6 years ago
Also, did you want me to try to put together some tests for this?
(Assignee)

Comment 7

6 years ago
Created attachment 530106 [details]
After patch - the warning on OSX

Is the warning enough?  Or should we find a way to style the drop-down to look more disabled?
(In reply to comment #6)
> Also, did you want me to try to put together some tests for this?

Yes please.
(Assignee)

Comment 9

6 years ago
Created attachment 530425 [details] [diff] [review]
Patch v2 - now with tests

Alright, added a couple of Mozmill tests for this.

I brought over a few testing functions I had originally written for bug 422845.  I'll make that bug depend on this and re-base, since this patch needs the quickness.
Attachment #530091 - Attachment is obsolete: true
Attachment #530425 - Flags: ui-review?(bwinton)
Attachment #530425 - Flags: review?(mbanner)
Attachment #530091 - Flags: review?(mbanner)
(Assignee)

Comment 10

6 years ago
Created attachment 530432 [details] [diff] [review]
Patch v3 - now with tests, and less cruft

Sorry for the bugspam.  Some unnecessary cruft in that last patch, which I've removed.

I'll give this a closer look tomorrow before I set the review flags.
Attachment #530425 - Attachment is obsolete: true
Attachment #530425 - Flags: ui-review?(bwinton)
Attachment #530425 - Flags: review?(mbanner)
(Reporter)

Updated

6 years ago
Whiteboard: [affects l10n][has proposed UI] → [affects l10n][has proposed UI][needs styling]
(Assignee)

Comment 11

6 years ago
Created attachment 531109 [details] [diff] [review]
Patch v4
Attachment #530432 - Attachment is obsolete: true
Attachment #531109 - Flags: review?(mbanner)
(Reporter)

Updated

6 years ago
Attachment #531109 - Flags: ui-review?(bwinton)
Comment on attachment 531109 [details] [diff] [review]
Patch v4

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

I feel like I want the dialog to be a little wider on OSX, so that the "You cannot change the address book because this contact is in one or more mailing lists." text doesn't look like it's poking out the sides.  Or maybe we just want to change the text to something a little shorter, like "You can't change the address book because the contact is in a mailing list."…

Next, the text in the address book should also probably be a disabled-grey if possible, or better might be to replace the dropdown with a label.

Finally, the Name/Email/Address Book labels should be right-aligned.

But, ui-r=me with those changes.

Thanks,
Blake.
Attachment #531109 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 13

6 years ago
Created attachment 531289 [details] [diff] [review]
Patch v5

Hm - I don't have a Mac to work on here at UDS, so while I can try to blindly implement those changes, I can't really test them. :/

I've updated the warning text to the shorter version that Blake suggested.

Also, a note on why I removed the editContactTextbox[readonly="true"] - notice how in the attachment 530092 [details], the readonly e-mail address is almost entirely invisible (ignore the part I blocked out manually - notice how "mike" and ".com" are almost completely unreadable.

So I removed that CSS so that it could be more readable - see attachment 530093 [details].
Attachment #531109 - Attachment is obsolete: true
Attachment #531289 - Flags: review?(mbanner)
Attachment #531109 - Flags: review?(mbanner)
(Reporter)

Comment 14

6 years ago
I'm currently looking at the Mac stuff, I've also got a few other changes that I'm trying out as well (including even shorter text ;-) ), should have a patch up in an hour or so.
(Reporter)

Comment 15

6 years ago
Created attachment 531299 [details]
Possible reworking on Mac
(Reporter)

Comment 16

6 years ago
Created attachment 531301 [details]
With the longer text
(Reporter)

Comment 17

6 years ago
Created attachment 531304 [details] [diff] [review]
Patch v6

I've done a few tweaks to the patch:

- The description for the warning is now displayed as part of the grid. Hence it only shows in the second column.
- The text is for the description is stored in the dtd and hence we don't need to be copying the string around all the time.
- Provided theming for Mac and Windows.
- Added end-alignment for the main labels.

I'm currently reviewing the test code and pushing this to try server.
Attachment #531289 - Attachment is obsolete: true
Attachment #531299 - Attachment is obsolete: true
Attachment #531289 - Flags: review?(mbanner)
(Reporter)

Comment 18

6 years ago
Created attachment 531316 [details] [diff] [review]
Patch v7

Uses GrayText rather than grey.
(Reporter)

Comment 19

6 years ago
Created attachment 531378 [details] [diff] [review]
Patch v8

Ok, this fixes an issue with the buttons getting shrunk on linux (we've added a vbox around the buttons which means they seem to work better when the new description gets wrapped).

It has my r+ for the bits Mike's done.
Attachment #531304 - Attachment is obsolete: true
Attachment #531316 - Attachment is obsolete: true
Attachment #531378 - Flags: ui-review?(bwinton)
Attachment #531378 - Flags: review?(bwinton)
(Reporter)

Comment 20

6 years ago
Created attachment 531381 [details]
Windows screenshot
(Reporter)

Comment 21

6 years ago
Created attachment 531402 [details]
Linux screenshot
Comment on attachment 531378 [details] [diff] [review]
Patch v8

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

For the UI-Review, on Mac the alignment is all over the place, and I would like to see that cleaned up (but I would be okay with part of that happening in a followup bug).  Other than that, I'm pretty happy with the UI.

And nothing jumped out at me for the review either, so I'll call it r=me, too.

Thanks,
Blake.
Attachment #531378 - Flags: ui-review?(bwinton)
Attachment #531378 - Flags: ui-review+
Attachment #531378 - Flags: review?(bwinton)
Attachment #531378 - Flags: review+
(Reporter)

Updated

6 years ago
Blocks: 656113
(Reporter)

Comment 23

6 years ago
Landed with a minor change to the alignment on Mac.

Filed bug 656113 on fixing the alignments up completely later.

Checked in: http://hg.mozilla.org/comm-central/rev/10b8d5efa321
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [affects l10n][has proposed UI][needs styling] → [affects l10n]
Target Milestone: --- → Thunderbird 3.3a4
You need to log in before you can comment on or make changes to this bug.