1183618 - Implement the refreshed design for the contacts list

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mike de Boer [:mikedeboer]]

As the acceptance criteria in bug 1179210 states:

 - Contact list visual refresh

Note: this _does_ include the individual contact entries and does _not_ include the presented favorites feature and related design elements.

For the visual design spec, please check out the ones attached to bug 1179210.

Comment 1

[Andrei Oprea [:andreio]]

I'm using https://bug1179210.bugzilla.mozilla.org/attachment.cgi?id=8630981 for the design of the contacts panel. That mockup does not include how audio-only conversations should be initiated.

Comment 2

[Mike de Boer [:mikedeboer]]

(In reply to Andrei Oprea [:andreio] from comment #1)
> I'm using https://bug1179210.bugzilla.mozilla.org/attachment.cgi?id=8630981
> for the design of the contacts panel. That mockup does not include how
> audio-only conversations should be initiated.

This is out of scope for this initial implementation. The specs contain various designs for features that have not been implemented yet, which is cool, but that doesn't mean we'll have to implement all these.
In fact, _everything_ that has not already been implemented should not be part of the visual refresh at this time.

IOW, please leave audio-only calls out of this bug.

Comment 3

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-20 at 10.01.03 AM.png

The scroll bar might make it a bit annoying to click the menu icon (shows up when you have > 6,7 contacts). But that does not show up at all times (at least on OSX default behavior is for it to hide). Moving menu icon further to the left makes it ugly when the scrollbar is not there, not sure how to proceed.

Comment 4

[Andrei Oprea [:andreio]]

Attachment: Implement refreshed design for the contacts list

Comment 5

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #3)
> Created attachment 8636083 [details]
> Screen Shot 2015-07-20 at 10.01.03 AM.png
> 
> The scroll bar might make it a bit annoying to click the menu icon (shows up
> when you have > 6,7 contacts). But that does not show up at all times (at
> least on OSX default behavior is for it to hide). Moving menu icon further
> to the left makes it ugly when the scrollbar is not there, not sure how to
> proceed.

If the bar is not always there, I know this is true in most OS but if it's true for all, just leave the options menu where it was originally stated.

Please look for the correct video camera icon in this link (inside the convo buttons folder)

On the other hand, can you please flag it with "ui-review" to me when you think is ready so I can check everything is OK from the visual side? Thanks!

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8636248 [details] [diff] [review]
Implement refreshed design for the contacts list

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

::: browser/components/loop/content/css/contacts.css
@@ +27,5 @@
>  
> +.contact-list-title {
> +  padding: 0 1rem;
> +  color: #666;
> +  text-transform: uppercase;

This isn't reliable for L10n use:

https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#CSS_issues

Please drop this from the css and make the value in loop.properties all uppercase. Also I suggest adding a l10n note along the lines that this is a title and they should do what's best for their locale.

@@ +96,5 @@
>  
> +/*
> + * Loop through all 12 default avatars.
> + */
> +.contact:nth-child(12n + 1) .avatar.defaultAvatar {

I think we should be able to use child selectors here - they are faster than descendant selectors.

@@ +97,5 @@
> +/*
> + * Loop through all 12 default avatars.
> + */
> +.contact:nth-child(12n + 1) .avatar.defaultAvatar {
> +  background-image: url("../shared/img/contacts_avatars/userthumb_blue.svg");

We should make these into one image file, with multiple shades. See icons-14x14.svg for an example (there's a -white fill).

::: browser/components/loop/content/js/contacts.jsx
@@ +614,5 @@
>              : null }
>              <GravatarPromo handleUse={this.handleUseGravatar}/>
>            </div>
> +          {shownContacts.available || shownContacts.blocked ?
> +          <div className="contact-list-title">

Please get UX review for the no-contacts case as well as the contacts case. I found it very weird and confusing to get a panel with a big empty section as I didn't have any contacts in the profile I was testing.

Comment 7

[Andrei Oprea [:andreio]]

This bug requires an empty contact list state text.

Comment 8

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #7)
> This bug requires an empty contact list state text.

Thanks for bringing this up Andrei, I will work on the empty state of both conversations and contacts list and tell you when ready.

Comment 9

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #7)
> This bug requires an empty contact list state text.

Visual spec update with this scenarios.

Comment 10

[Sevaan Franks [:sevaan]]

Hi Matej,

When users first open Firefox Hello, if they do not have any conversations or contacts, we are currently displaying an empty panel. We feel some sort of visual or a string should be displayed instead indicating the empty nature of the room.

In the mockups the current strings are...

Contact panel: http://i.sevaan.com/image/1w1D2g2o391b
Conversation panel: http://i.sevaan.com/image/3c0m012L2G3T

Could you please review?

Comment 11

[Matej Novak [:matej]]

WDYT of these?

No contacts yet
Import or add someone

No open conversations
Start one now!

In the second one, "open" could also be "active." I didn't use "yet" there because I imagined it could also be empty because the user deleted all their conversations.

Comment 12

[Sevaan Franks [:sevaan]]

Matej, how about just "No Conversations"?

Open or Active to me implies in use...and you could have some rooms in the list you are not using at the moment, so their would be neither open or active.

Comment 13

[Matej Novak [:matej]]

(In reply to Sevaan Franks [:sevaan] from comment #12)
> Matej, how about just "No Conversations"?
> 
> Open or Active to me implies in use...and you could have some rooms in the
> list you are not using at the moment, so their would be neither open or
> active.

I see what you mean, but "no conversations" seems a bit terse. Maybe "No conversations here" is a little more playful?

Comment 14

[Sevaan Franks [:sevaan]]

I like it.

Andrei, let's use these strings:

No contacts yet
Import or add someone

No conversations here
Start one now!

Vicky, do you want to update your mockups to reflect this?

Comment 15

[Andrei Oprea [:andreio]]

Attachment: Implement refreshed design for the contacts list

Comment 16

[Andrei Oprea [:andreio]]

Comment on attachment 8637497 [details] [diff] [review]
Implement refreshed design for the contacts list

I included the mozLoop prop in the component to that we may stub mozLoop object and have several variations of the ContactList component in the showcase.

Comment 17

[Andrei Oprea [:andreio]]

Attachment: Implement refreshed design for the contacts list

Comment 18

[Victoria Gerchinhoren [:vicky]]

(In reply to Sevaan Franks [:sevaan] from comment #14)
> I like it.
> 
> Andrei, let's use these strings:
> 
> No contacts yet
> Import or add someone
> 
> No conversations here
> Start one now!
> 
> Vicky, do you want to update your mockups to reflect this?

If the new strings are stated here, there's no need to update mocks, updating hi-fi designs all the time is too time consuming, that should be already stated and revised in WF.

Comment 19

[Victoria Gerchinhoren [:vicky]]

Also, we talked many times about a copy reviewing all the UI strings, maybe it is a good time for a professional to jump in, not only to solve a few, but to give general recommendations about the overall TOV of the service... Don't you think?

Comment 20

[Victoria Gerchinhoren [:vicky]]

There's an update on the file, showing how the "no matching results" screen would look like, assets in the respective assets folder. Cheers.

Comment 21

[Sevaan Franks [:sevaan]]

(In reply to Victoria Gerchinhoren [:vicky] from comment #18)
> (In reply to Sevaan Franks [:sevaan] from comment #14)
> If the new strings are stated here, there's no need to update mocks,
> updating hi-fi designs all the time is too time consuming, that should be
> already stated and revised in WF.

I thought you might want to keep the Sketch file up to date, so that we always have a replica of what's in Firefox in Sketch format.

Comment 22

[Sevaan Franks [:sevaan]]

(In reply to Victoria Gerchinhoren [:vicky] from comment #19)
> Also, we talked many times about a copy reviewing all the UI strings, maybe
> it is a good time for a professional to jump in, not only to solve a few,
> but to give general recommendations about the overall TOV of the service...
> Don't you think?

How do we best facilitate this?

Comment 23

[Romain Testard [:RT]]

Just pinged matej on https://bugzilla.mozilla.org/show_bug.cgi?id=1179163#c2 for an overall copy review.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 8637629 [details] [diff] [review]
Implement refreshed design for the contacts list

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

This looks better, but I'm going to pass the review to Dan as I've run out of time this week.

Comment 25

[Andrei Oprea [:andreio]]

I'm not sure we have an ellipsis icon (the one on the right that expands the menu for each contact).

Comment 26

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #25)
> I'm not sure we have an ellipsis icon (the one on the right that expands the
> menu for each contact).

There.

Comment 27

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #25)
> I'm not sure we have an ellipsis icon (the one on the right that expands the
> menu for each contact).

There.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8637629 [details] [diff] [review]
Implement refreshed design for the contacts list

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

This is looking good.  One thing that needs fixing is that in RTL mode the vertical ellipsis placing is wrong.  This is otherwise mostly small comments, and some things that can be spun off.  
I think I only need to see this again if there are substantive changes in ui-review, once you've addressed the following things somehow:

* I don't think this implements fading that last visible contact in the viewport while scrolling as shown by the mock, but it's hard to tell because there's no ui-showcase view which displays that.  Spinning off a new bug for this sounds fine.

* The Import/Add New Contact buttons need to move; if that's not in another bug already, please spin one out.

* the per contact drop down menu doesn't look much like the mock (it looks better in some ways; different in others), and it's not touched by this patch.  If you can't find another bug where this is happening, please spin one off.  Here are some bullet points that I noticed that you can put in that bug:

** put menu itself in ui-showcase since it’s not immediately visible without clicking?
** last contact has edit & remove grayed out
** why are video and audio call items still in menu?
** has icons on the left side, the mock doesn't
** in the ui-showcase it extends upward to be occluded by the contact-list title

r=dmose, modulo any substantial changes.

::: browser/components/loop/content/css/contacts.css
@@ +160,5 @@
>    width: 100%;
>  }
>  
> +.contact-list-empty {
> +  background-image: url("../shared/img/empty_contacts.svg");

If it's easy, I'd prefer a pseudo-element or <img>; since this isn't a background, it's the primary content.  Given the amount of stuff to do, just mentioning that in an XXX would be fine.

@@ +181,5 @@
> +}
> +
> +.panel-text-large {
> +  font-size: 22px;
> +}

Presumably the font-sizes want to be specified in rems now.

Maybe the margin also; I'll leave that up to you.

::: browser/components/loop/content/js/contacts.jsx
@@ +322,2 @@
>                 onClick={this.handleAction.bind(null, "video-call")} />
> +            <i className="icon icon-ellipsis-down"

icon-vertical-ellipsis would be a more broadly understood classname, I think.

::: browser/components/loop/content/shared/img/avatars.svg
@@ +17,5 @@
> +      fill: #fff;
> +    }
> +  </style>
> +  <defs>
> +    <g id="blue" transform="translate(-2588 -413) translate(2588 413)" fill="none">

That's an odd sort pair of translates.  That said, since this is machine generated, I don't think it's a good use of time to go hand-optimizing this stuff.

::: browser/components/loop/modules/MozLoopAPI.jsm
@@ +899,4 @@
>       * Compose a URL pointing to the location of an avatar by email address.
>       * At the moment we use the Gravatar service to match email addresses with
>       * avatars. This might change in the future as avatars might come from another
>       * source.

So I assume that the order of precedence here is FxA image falling back to Gravatar.  If that's true, can you please make the jsdoc comment explicit?

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +165,5 @@
>        checkGravatarContacts(false);
>      });
>  
>      it("should not show the gravatars promo box when the 'contacts.gravatars.promo' pref is set", function() {
> +      sandbox.stub(navigator.mozLoop, "getLoopPref", function(pref) {

Nice use of sandbox.stub; I hadn't seen this form before.

@@ +270,5 @@
> +            mozLoop: navigator.mozLoop,
> +            notifications: notifications,
> +            startForm: function() {}
> +          }));
> +          node = listView.getDOMNode();

This is hard to read; I suspect it wants to be outdented?  Same problem in later clauses.

::: browser/components/loop/ui/fake-mozLoop.js
@@ +145,1 @@
>      },

I'm not seeing any gravatar URLs in the showcase at all; is that expected?

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +128,5 @@
>  ## LOCALIZATION NOTE(sync_contacts_button): This button is displayed in place of
>  ## importing_contacts_button once contacts have been imported once.
>  sync_contacts_button=Sync Contacts
> +no_contacts_message_heading=No contacts yet
> +no_contacts_message_subheading=Import or add someone

How about naming the second string "no_contacts_import_or_add" for l10n clarity.  A localization note explaining the context of what these two strings are in a bit more detail would be great.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: implement the refreshed design for the contacts list

Updated patch rebased against current fx-team

Comment 30

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-31 at 12.16.05 AM.png

Comment 31

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-31 at 12.16.12 AM.png

Comment 32

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8641530 [details]
Screen Shot 2015-07-31 at 12.16.12 AM.png

The icons are not the provided ones, can you point me where are you getting this from? The ones i have provided are in the assets folder in Dropbox, but it might be good to track where are the icons used in other Firefox panels...

Comment 33

[Andrei Oprea [:andreio]]

Haven't noticed that the contacts dropdown panel has no icons but the conversation one does. Is this intended?

Comment 34

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-08-03 at 7.47.29 AM.png

Comment 35

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #33)
> Haven't noticed that the contacts dropdown panel has no icons but the
> conversation one does. Is this intended?

Hey Andrei, good catch, this 2 panels come from different specs so, I haven't noticed either. I updated the mockups with the icons on it and you can find the assets in the Panel UI folder inside here: https://www.dropbox.com/sh/jjg70il7lzftpng/AACsVTcvM9f6bFxCxsjsHaJ8a?dl=0

Comment 36

[Andrei Oprea [:andreio]]

Do we have an "unblock" icon?

Comment 37

[Sevaan Franks [:sevaan]]

Can I suggest we don't use any icons to better align with the look of other secondary menus in Firefox (we don't use icons in any other menu, so will probably remove the icons in the future anyway).

Comment 38

[Andrei Oprea [:andreio]]

I was also unable to find the vertical ellipsis icon which expands the contact drop down (from the contacts list).

Comment 39

[Andrei Oprea [:andreio]]

Clearing needinfo. Vertical ellipsis still relevant.

Comment 40

[Andrei Oprea [:andreio]]

Attachment: Implement refreshed design for the contacts list

Comment 41

[Victoria Gerchinhoren [:vicky]]

Agree with Sevaan, let's take all icons away but the "available / do not disturb" ones yet. 

The ellipsis icon is in the Panel UI assets folder.

Comment 42

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8642430 [details]
Screen Shot 2015-08-03 at 7.47.29 AM.png

Thanks Andrei.

Please adjust the following:

- Use the provided ellipsis icon.

- Line height for the text inside the menu should be 24px

- add top and bottom padding inside the menu box, at least 5px more.

- increase the left margin of the list, make it 15px (take a look at the visual specs for the panel UI)

- use the provided camera icon for the rounded button.


Also, it's a bit tricky to review the screenshots in a greater size. Can you please upload 100% view? Thanks!

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8642667 [details] [diff] [review]
Implement refreshed design for the contacts list

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

r=dmose, after you've spun off a bug to move the PanelViews in the ui-showcase to FramedExamples so that they render avoid cropping stuff improperly, and probably otherwise render more accurately too.

Comment 44

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-08-04 at 4.06.08 PM.png

Comment 45

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-08-04 at 4.05.53 PM.png

Comment 46

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-08-04 at 3.45.49 PM.png

Comment 47

[Andrei Oprea [:andreio]]

I could not find the camera icon I need to use so if you could point me to it that would be great, thank you. 

I addressed your review comments so if something is off I might not have understood what is required.

Comment 48

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8643352 [details]
Screen Shot 2015-08-04 at 4.06.08 PM.png

Sorry Andrei, this still looks a bit off. Please adjust:

Text color: #4a4a4a
Text size: 12 px
Line height: 24px
Higlight color: #DBF7FF

Camera icon is inside the assets folder > Convo_buttons.

Thanks

Comment 49

[Andrei Oprea [:andreio]]

I hope I got it right this time :)

If the screenshot size is wrong try zooming out I saw it is possible in the dropbox interface after you click the image it shows it in "true size".

https://www.dropbox.com/s/knu8y7cdla06co8/Screenshot%202015-08-05%2019.23.48.png?dl=0

https://www.dropbox.com/s/60634h3ydh6s8xj/Screenshot%202015-08-05%2019.23.58.png?dl=0

Comment 50

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #49)
> I hope I got it right this time :)
> 
> If the screenshot size is wrong try zooming out I saw it is possible in the
> dropbox interface after you click the image it shows it in "true size".
> 
> https://www.dropbox.com/s/knu8y7cdla06co8/Screenshot%202015-08-05%2019.23.48.
> png?dl=0
> 
> https://www.dropbox.com/s/60634h3ydh6s8xj/Screenshot%202015-08-05%2019.23.58.
> png?dl=0

They look perfect! Thanks Andrei ;)

Comment 51

[Andrei Oprea [:andreio]]

https://hg.mozilla.org/integration/fx-team/rev/9eafb1464913

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9eafb1464913

Comment 53

[Bogdan Maris, Desktop Test Engineering]

Verified using latest Aurora 42.0a2 across platforms (Windows 7 64-bit, Mac OS X 10.10.4 and Ubuntu 14.04 32-bit) that the refreshed design for contacts list matches the implementation here. No new issues were found during testing.

Comment 54

[Mark Banner (:standard8)]

Attachment: Backout for Aurora (42)

Approval Request Comment
[Feature/regressing bug #]: FF Hello Visual Refresh, backout patch 2 of 5
[User impact if declined]: Partial refresh implementation landed for 42, but is now being completed for 43 and isn't suitable for uplift. Hence, the current partially done 42 has bad layouts and an incomplete design change.
[Describe test coverage new/current, TreeHerder]: Code has unit tests
[Risks and why]: Low - reverting to the previous version.
[String/UUID change made/needed]: None. Any string changes were not backed out.

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Mark, could you also report another bug for this backout? Thanks

Comment 56

[Sylvestre Ledru [:Sylvestre]]

Actually, a bug for all the backouts would be fine.

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8658614 [details] [diff] [review]
Backout for Aurora (42)

managed in bug 1203454

Comment 58

[Mark Banner (:standard8)]

Clearing obsolete NI