Closed
Bug 1084296
Opened 10 years ago
Closed 9 years ago
Add an ellipsis to Loop's "Remove Contact" menu item
Categories
(Hello (Loop) :: Client, defect, P4)
Hello (Loop)
Client
Tracking
(firefox40 fixed)
People
(Reporter: cvan, Assigned: tomesh, Mentored)
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(2 files, 1 obsolete file)
93.96 KB,
image/png
|
Details | |
3.58 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
1. Click the Loop talk icon. 2. Click the person icon in the top nav (make sure you are first signed in). 3. Hover over a contact (press the "Add a Contact" first if you have none). 4. Carefully click the white down-arrow icon in the green button that appears. 5. Notice "Edit Contact…" is the only item in the menu that ends with an ellipsis. I understand using an ellipsis if doing `text-overflow: ellipsis`, but there's certainly room here (and it's not even the longest string). So I think the ellipsis should be removed for consistency's sake.
Comment 1•10 years ago
|
||
There's a general rule in Firefox UI about ellipsis, and I believe its along the lines of "If clicking the menu item doesn't complete the action on its own (but further input is required), then it should use an ellipsis". I'm pretty sure that's right, but needinfo Darrin.
Flags: needinfo?(dhenein)
Comment 2•10 years ago
|
||
This is correct, the ellipsis should remain. That being said, I believe Remove Contact should gain an ellipsis as well (as there is a confirm dialog upon selecting).
Flags: needinfo?(dhenein)
Comment 3•10 years ago
|
||
Thanks Darrin, updating bug title.
Severity: normal → minor
OS: Mac OS X → All
Hardware: x86 → All
Summary: Loop's "Edit Contact…" menu item should not end with an ellipsis → Add an ellipsis to Loop's "Remove Contact" menu item
Updated•10 years ago
|
backlog: --- → Fx36?
Comment 4•10 years ago
|
||
will need to go through all UX bugs against each other
backlog: Fx36? → backlog
Whiteboard: [UX]
Updated•9 years ago
|
backlog: backlog+ → backlog-
Comment 5•9 years ago
|
||
Marking as good first bug. To fix this bug, the "remove_contact_menu_button" string needs an ellipsis adding to it. Additionally, its best to change the string id to "remove_contact_menu_button2" to signal to l10n about the change. The locations to change can be found here: http://mxr.mozilla.org/mozilla-central/search?string=remove_contact_menu_button&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central More info about Loop development is here: https://wiki.mozilla.org/Loop/Development
Mentor: standard8
Rank: 48
Flags: qe-verify-
Flags: firefox-backlog+
Priority: -- → P4
Whiteboard: [UX] → [good first bug][lang=js]
Assignee | ||
Comment 6•9 years ago
|
||
Hello, I'd like to be assigned to this bug. Could someone assign me, please?
Comment 7•9 years ago
|
||
(In reply to Martin Tomes [:tomesh] from comment #6) > Hello, I'd like to be assigned to this bug. Could someone assign me, please? Yes, of course. If you need any help feel free to ask.
Assignee: nobody → tomesm
Assignee | ||
Comment 8•9 years ago
|
||
Add ellipsis and change remove string ID in contacts.js, contacts.jsx and loop.properties.
Attachment #8599254 -
Flags: review?(standard8)
Comment 9•9 years ago
|
||
Comment on attachment 8599254 [details] [diff] [review] rev1 - Add ellipsis and change remove_contact_menu_button string ID Review of attachment 8599254 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +128,5 @@ > import_failed_support_button=Help > > ## LOCALIZATION NOTE(remove_contact_menu_button): Displayed in the contact list in > ## a pop-up menu next to the contact's name. > +remove_contact_menu_button2=Remove Contact... Almost right, but not quite. We use the ellipsis character '…' rather than three dots. If you look at contacts_search_placesholder, there's one you can copy and paste.
Attachment #8599254 -
Flags: review?(standard8) → review-
Comment 10•9 years ago
|
||
Comment on attachment 8599254 [details] [diff] [review] rev1 - Add ellipsis and change remove_contact_menu_button string ID Review of attachment 8599254 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +128,5 @@ > import_failed_support_button=Help > > ## LOCALIZATION NOTE(remove_contact_menu_button): Displayed in the contact list in > ## a pop-up menu next to the contact's name. > +remove_contact_menu_button2=Remove Contact... Ooh, I just remembered, please update the string name in the LOCALIZATION NOTE as well.
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8599266 -
Flags: review?(standard8)
Updated•9 years ago
|
Attachment #8599254 -
Attachment is obsolete: true
Comment 12•9 years ago
|
||
Comment on attachment 8599266 [details] [diff] [review] rev2 - an ellipsis character added an LOCALIZATION NOTE string updated Review of attachment 8599266 [details] [diff] [review]: ----------------------------------------------------------------- That looks great. Thanks for the patch. My only comment is for the checkin message, I'd keep it the same as the bug title - no need to detail which files are being changed, as the changeset itself does that. Don't worry about changing the commit message here. I've got the patch in my queue ready for landing with the updated title. I'll land it as soon as the tree re-opens (I've got another patch to land anyway). The alternative for landing would be to use checkin-needed, but since that requires a push to the try servers, I'm happy to just land this straight away for you.
Attachment #8599266 -
Flags: review?(standard8) → review+
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #12) > Comment on attachment 8599266 [details] [diff] [review] > rev2 - an ellipsis character added an LOCALIZATION NOTE string updated > > Review of attachment 8599266 [details] [diff] [review]: > ----------------------------------------------------------------- > > That looks great. Thanks for the patch. > > My only comment is for the checkin message, I'd keep it the same as the bug > title - no need to detail which files are being changed, as the changeset > itself does that. > > Don't worry about changing the commit message here. I've got the patch in my > queue ready for landing with the updated title. I'll land it as soon as the > tree re-opens (I've got another patch to land anyway). > > The alternative for landing would be to use checkin-needed, but since that > requires a push to the try servers, I'm happy to just land this straight > away for you. Thanks for your comments and hints. I will keep them in mind for the next time :) It is my first bug and I enjoyed the contribution. Im looking forward to further collaboration :) As I do not have permissions I suppose from my side the work on this bug is finished.
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e608444e72aa
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•