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.

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.

(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.

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.

(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 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.

This bug requires an empty contact list state text.

(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.

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

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?

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.

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.

(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?

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 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.

(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.

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?

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

(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.

(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?

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

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.

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

(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.

(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 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.

Updated patch rebased against current fx-team

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...

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

(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

Do we have an "unblock" icon?

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).

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

Clearing needinfo. Vertical ellipsis still relevant.

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 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 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.

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 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

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

(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 ;)

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.

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.

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

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

Comment on attachment 8658614 [details] [diff] [review] Backout for Aurora (42) managed in bug 1203454

Clearing obsolete NI