Closed
Bug 495937
Opened 15 years ago
Closed 14 years ago
New Smart Folder dialog doesn't use a verb for the confirm button
Categories
(Thunderbird :: Mail Window Front End, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.3a1
People
(Reporter: andreasn, Assigned: ewong)
Details
Attachments
(3 files, 5 obsolete files)
36.48 KB,
image/png
|
Details | |
77.20 KB,
image/png
|
Details | |
4.94 KB,
patch
|
ewong
:
review+
standard8
:
superreview+
clarkbw
:
ui-review+
ewong
:
feedback+
|
Details | Diff | Splinter Review |
According to the Interface guidelines for both Vista, OS X and GNOME, it's preferred to use a verb instead of a standard button labeled OK. I would suggest something like "Create", "Create folder", "Create new saved search" (a bit too long maybe). Bryan, what would you suggest here?
Comment 1•15 years ago
|
||
I tend to go shorter when the item is complex, so I'd suggest 'Create'
Updated•14 years ago
|
Assignee: nisses.mail → nobody
Severity: normal → trivial
Component: General → Mail Window Front End
QA Contact: general → front-end
Comment 2•14 years ago
|
||
Comment 3•14 years ago
|
||
philor, have time for this?
Attachment #425663 -
Flags: review?(philringnalda)
Comment 4•14 years ago
|
||
Edmund, maybe you want to take this one over as well? The patch needs some work to make an accesskey happen
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #4) > Edmund, maybe you want to take this one over as well? The patch needs some > work to make an accesskey happen Sure.
Assignee: nobody → edmund
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #4) > Edmund, maybe you want to take this one over as well? The patch needs some > work to make an accesskey happen Btw, I'm using "R" as the accesskey. It looks ok, though confirmation from you or anyone would be appreciated.
Comment 7•14 years ago
|
||
Comment on attachment 425663 [details] [diff] [review] changes accept button to use the verb "Create" removing this review for now since Edmund is working on a new patch
Attachment #425663 -
Flags: review?(philringnalda)
Comment 8•14 years ago
|
||
(In reply to comment #6) > (In reply to comment #4) > > Edmund, maybe you want to take this one over as well? The patch needs some > > work to make an accesskey happen > > Btw, I'm using "R" as the accesskey. It looks ok, though confirmation > from you or anyone would be appreciated. R sounds good, I'm assuming C will already be taken by the Cancel
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #432106 -
Flags: review?(bugzilla)
Assignee | ||
Updated•14 years ago
|
Attachment #432106 -
Attachment description: Changed Ok button to Create, with R as access key. → Changed Ok button to Create, with R as access key and added License block to virtualFolderProperties.dtd.
Comment 10•14 years ago
|
||
Comment on attachment 432106 [details] [diff] [review] Changed Ok button to Create, with R as access key and added License block to virtualFolderProperties.dtd. I had a quick discussion with Bryan about this over irc. The unfortunate side-effect of this patch is that for editing saved searches, the button to accept will also be called "Create". Which, of course, isn't quite right for editing. Bryan and I suggest using "Update" for the edit case.
Attachment #432106 -
Flags: review?(bugzilla) → review-
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #10) > (From update of attachment 432106 [details] [diff] [review]) > I had a quick discussion with Bryan about this over irc. > > The unfortunate side-effect of this patch is that for editing saved searches, > the button to accept will also be called "Create". Which, of course, isn't > quite right for editing. > > Bryan and I suggest using "Update" for the edit case. Makes sense. I'll have the patch done soon. While I'm at it, what about the part about the license block? Should I also include it or leave it out?
Assignee | ||
Comment 12•14 years ago
|
||
This patch is without the license block addition.
Attachment #432106 -
Attachment is obsolete: true
Attachment #435347 -
Flags: review?(bugzilla)
Comment 13•14 years ago
|
||
Comment on attachment 435347 [details] [diff] [review] Changed Ok button to Update, with U as access key. Unfortunately I don't think this is quite what we intended. I think this dialog needs to be "Create" when creating a new folder, and "Update" when editing an existing one. Hence you need two labels and to switch between them according to how the dialog is opened. We do do this in other places.
Attachment #435347 -
Flags: review?(bugzilla) → review-
Assignee | ||
Comment 14•14 years ago
|
||
The button becomes Create if it's from a "New" instruction. It becomes "Update" if it is from a "Properties" instruction.
Attachment #425663 -
Attachment is obsolete: true
Attachment #435347 -
Attachment is obsolete: true
Comment 15•14 years ago
|
||
Comment on attachment 444109 [details] [diff] [review] Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened. (In reply to comment #14) > Created an attachment (id=444109) [details] > Changed the Smart Folder button to reflect its function depending on how the > Smart Folder is opened. > > The button becomes Create if it's from a "New" instruction. It becomes > "Update" if it is from a "Properties" instruction. This looks pretty close! I think you just need to make those strings localizable. Take a look at the code in this file: http://mxr.mozilla.org/comm-central/source/mail/extensions/mailviews/content/mailViewList.js#89 You need a string bundle like this: var strBundleService = Components.classes["@mozilla.org/intl/stringbundle;1"]. getService(Components.interfaces.nsIStringBundleService); var bundle = strBundleService.createBundle("chrome://messenger/locale/messenger.properties"); Then you could set the label like this: document.documentElement.getButton("accept").label = bundle.GetStringFromName("editExistingFolderAcceptLabel") Which allows for labels of different languages inside the javascript code.
Assignee | ||
Comment 16•14 years ago
|
||
(In reply to comment #15) > This looks pretty close! I think you just need to make those strings > localizable. > > You need a string bundle like this: > > var strBundleService = > Components.classes["@mozilla.org/intl/stringbundle;1"]. > getService(Components.interfaces.nsIStringBundleService); > var bundle = > strBundleService.createBundle("chrome://messenger/locale/messenger.properties"); > > Then you could set the label like this: > > document.documentElement.getButton("accept").label = > bundle.GetStringFromName("editExistingFolderAcceptLabel") > > Which allows for labels of different languages inside the javascript code. Just wondering. What about the accesskey? Do they need some sort of localizable setting too?
Comment 17•14 years ago
|
||
Actually what we tend to do in situations like this is to define the labels and access keys in the dtd file and reference them in attributes of the dialog. So something like existingAcceptLabel="&editExistingFolderAccept.label;" newAcceptLabel="&newFolderAccept.label;" and then in the js just use getButton("accept").label = dialog.newAcceptLabel (the code isn't quite right, but hopefully you get the idea).
Assignee | ||
Comment 18•14 years ago
|
||
Attachment #444109 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #444109 -
Flags: feedback?(bugspam.Callek)
Assignee | ||
Updated•14 years ago
|
Attachment #444411 -
Flags: review?(iann_bugzilla)
Attachment #444411 -
Flags: feedback?(bugspam.Callek)
Assignee | ||
Updated•14 years ago
|
Attachment #444109 -
Flags: feedback?(bugspam.Callek)
Assignee | ||
Updated•14 years ago
|
Attachment #444411 -
Flags: ui-review?(clarkbw)
Attachment #444411 -
Flags: superreview?(bugzilla)
Comment 19•14 years ago
|
||
Comment on attachment 444411 [details] [diff] [review] Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened. (v2) Code looks good, one (very minor nit: set the accept label to Update from the start, so that something more appropriate than "OK" shows in the case of a slow computer not running onload faster than the human eye scans; -- don't change this unless a "real" reviewer wants it though)
Attachment #444411 -
Flags: feedback?(bugspam.Callek) → feedback+
Comment 20•14 years ago
|
||
Comment on attachment 444411 [details] [diff] [review] Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened. (v2) >+++ b/mailnews/base/content/virtualFolderProperties.js I'd prefer setting a variable to document.documentElement.getButton("accept") then using that in the if statement. > if (arguments.editExistingFolder) >+ { >+ document.documentElement.getButton("accept").label = >+ document.documentElement.getAttribute("editFolderAcceptButtonLabel"); >+ document.documentElement.getButton("accept").accesskey = >+ document.documentElement.getAttribute("editFolderAcceptButtonAccessKey"); > InitDialogWithVirtualFolder(arguments.folder); >+ } > else // we are creating a new virtual folder > { >+ document.documentElement.getButton("accept").label = >+ document.documentElement.getAttribute("newFolderAcceptButtonLabel"); >+ document.documentElement.getButton("accept").accesskey = >+ document.documentElement.getAttribute("newFolderAcceptButtonAccessKey"); so r=me with the above addressed.
Attachment #444411 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Comment 21•14 years ago
|
||
r+IanN, f+Callek
Attachment #444411 -
Attachment is obsolete: true
Attachment #445953 -
Flags: review+
Attachment #445953 -
Flags: feedback+
Attachment #444411 -
Flags: ui-review?(clarkbw)
Attachment #444411 -
Flags: superreview?(bugzilla)
Assignee | ||
Updated•14 years ago
|
Attachment #445953 -
Flags: ui-review?(clarkbw)
Attachment #445953 -
Flags: superreview?(bugzilla)
Comment 22•14 years ago
|
||
Comment on attachment 445953 [details] [diff] [review] Changed the Smart Folder button to reflect its function depending on how the Smart Folder is opened. (v3) Looks great, thanks.
Attachment #445953 -
Flags: superreview?(bugzilla) → superreview+
Updated•14 years ago
|
Attachment #445953 -
Flags: ui-review?(clarkbw) → ui-review+
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 23•14 years ago
|
||
Checked in: http://hg.mozilla.org/comm-central/rev/b5afb60824f3 Thanks Edmund.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Summary: New Smart Folder dialog don't use a verb for the confirm button → New Smart Folder dialog doesn't use a verb for the confirm button
Target Milestone: --- → Thunderbird 3.2a1
You need to log in
before you can comment on or make changes to this bug.
Description
•