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)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed
backlog backlog-

People

(Reporter: cvan, Assigned: tomesh, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 1 obsolete file)

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.
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)
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)
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
backlog: --- → Fx36?
will need to go through all UX bugs against each other
backlog: Fx36? → backlog
Whiteboard: [UX]
backlog: backlog+ → backlog-
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]
Hello, I'd like to be assigned to this bug. Could someone assign me, please?
(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
Add ellipsis and change remove string ID in contacts.js, contacts.jsx and loop.properties.
Attachment #8599254 - Flags: review?(standard8)
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 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.
Attachment #8599254 - Attachment is obsolete: true
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/e608444e72aa
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.