As the acceptance criteria in bug 1179210 states: - Search bar visual refresh For the visual design spec, please check out the ones attached to bug 1179210.

Comment on attachment 8634987 [details] [diff] [review] Implement the refreshed design for the contacts filter input Review of attachment 8634987 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, but needs some tweaks. I'd like to have another look before we land... Before this lands, we need to either get real assets from mmaslaney, or convince ourselves that the Font Awesome license works, which will require a needinfo on :gerv, as well as probably attribute the source of the SVG icons. Alternately, perhaps mmaslaney can point us at stock icons that are known to be license compatible while we're waiting for the real ones. Please update the UI-showcase to have more than 7 contacts so that this widget shows up in the showcase. You could consider splitting out "ShortContactList" and "LongContactList" views in the showcase. ::: browser/components/loop/content/css/contacts.css @@ +13,5 @@ > } > > +.contact-filter-container { > + position: relative; > +} I'm going to wait to review the CSS until the UI showcase stuff is here. Comments about the motivation of things (like why the container is position-relative will improve readability/maintainability. ::: browser/components/loop/content/js/contacts.jsx @@ +612,5 @@ > // consistent ordering. > return contact1._guid - contact2._guid; > }, > > + _renderContactsFilter: function() { If it's straightforward (eg timebox this to 30 or 60 mins or something), I'd suggest pulling ContactsFilter into its own React component for improved maintainability. @@ +621,5 @@ > + placeholder={mozL10n.get("contacts_search_placesholder")} > + valueLink={this.linkState("filter")} /> > + {this.state.filter ? <button className="clear-search" > + onClick={this._handleFilterClear} /> > + : null} Consider pulling this out into a function and dropping the ternary for improved readability. ::: browser/components/loop/test/desktop-local/contacts_test.js @@ +297,5 @@ > + > + expect(filterView).to.eql(null); > + }); > + > + it("should filter the user name correctly", function() { "a non-existent user name" @@ +321,5 @@ > + getAll: function(callback) { > + callback(null, [].concat(fakeManyContacts)); > + } > + } > + listView.refresh(); It seems like it would prevent unintended test interactions to simply create a new listView for each test rather than reusing an existing one. ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +88,5 @@ > # Contact Strings (Panel) > > ## LOCALIZATION NOTE(contacts_search_placeholder): This is the placeholder text for > ## the search field. > +contacts_search_placesholder=Type in contact name, email, phone number Please update the name here (maybe "contacts_search_placeholder") so that localizers know to re-localize. I have some concerns about this string; I'll do a needinfo...

Where can we find the assets from the mockups? (search magnifying glass, clear button etc.)

The string for the contacts search box placeholder: "Type in contact name, email, phone number" Doesn't feel quite right to me. To some degree, this is grammatical, e.g. I wonder if we might want to drop the words "Type in", since this seems unusual for placeholders. Needinfo on sevaan, vicky, and matej for their thoughts...

Maybe "enter" instead of "type in"? I agree that it feels odd.

(In reply to Andrei Oprea [:andreio] from comment #3) > Where can we find the assets from the mockups? (search magnifying glass, > clear button etc.) Please find assets here: https://www.dropbox.com/sh/0f7imuid7f99lse/AAB7VHOZrCGzIhFe7cgYdAN6a?dl=0 All svg files, but you can also choose to use the font awesome icon font for most of them.

> "Type in contact name, email, phone number" Do we need to specify all those things? We could just have it say "Search".

Comment on attachment 8637624 [details] [diff] [review] Implement the refreshed design for the contacts filter input Unclear to me what the decision is for the placeholder text. Will update afterwards.

Comment on attachment 8637624 [details] [diff] [review] Implement the refreshed design for the contacts filter input Review of attachment 8637624 [details] [diff] [review]: ----------------------------------------------------------------- r=dmose with the suggested tweaks, as appropriate. ::: browser/components/loop/content/css/contacts.css @@ +23,5 @@ > + padding-left: 2.5em; > + width: 100%; > + border: 0; > + border-bottom: 1px solid #ddd; > + background-image: url("../shared/img/search.svg"); Let's rename this to magnifying-glass-icon.svg. @@ +38,5 @@ > + right: 0.4em; > + width: 1.5em; > + height: 1.5em; > + border: none; > + background-image: url("../shared/img/times-circle.svg"); Let's rename this to search-clear-x-button.svg ::: browser/components/loop/test/desktop-local/contacts_test.js @@ +326,5 @@ > + navigator.mozLoop.contacts = { > + getAll: function(callback) { > + callback(null, [].concat(fakeManyContacts)); > + }, > + on: sandbox.stub() Nit: could make this a function since we're not actually using it for asserts.

Comment on attachment 8637624 [details] [diff] [review] Implement the refreshed design for the contacts filter input Review of attachment 8637624 [details] [diff] [review]: ----------------------------------------------------------------- The SVG sizes are really odd here... what happens if you make the viewbox 10x10 and 16x16 respectively? Because if they look good in that size, you can put the in the SVG icon maps _and_ the ui-showcase.

UI Review for Contacts filter input https://dl-web.dropbox.com/get/Screenshots/Screenshot%202015-08-06%2018.36.56.png?_subject_uid=453014287&w=AABbkpe6iwZi6YtLMcVNYPCSf_nV6Agpl4NZB7tWqEyekg Note: add contact and import buttons dissapear, MY CONTACTS title above contacts disappears, Gravatar image import disappears. https://dl-web.dropbox.com/get/Screenshots/Screenshot%202015-08-06%2018.37.06.png?_subject_uid=453014287&w=AACk8HoeQS8RmT4jN5fi4KR6ziL42KrGK3AKqqTsR5ifJw https://dl-web.dropbox.com/get/Screenshots/Screenshot%202015-08-06%2018.37.16.png?_subject_uid=453014287&w=AABTWaKebxEgiyt6I7V_iUp0b_79Uo0aE42jeG_-uRRAUA

(In reply to Andrei Oprea [:andreio] from comment #12) > UI Review for Contacts filter input > > https://dl-web.dropbox.com/get/Screenshots/Screenshot%202015-08-06%2018.36. > 56. > png?_subject_uid=453014287&w=AABbkpe6iwZi6YtLMcVNYPCSf_nV6Agpl4NZB7tWqEyekg > > Note: add contact and import buttons dissapear, MY CONTACTS title above > contacts disappears, Gravatar image import disappears. > https://dl-web.dropbox.com/get/Screenshots/Screenshot%202015-08-06%2018.37. > 06. > png?_subject_uid=453014287&w=AACk8HoeQS8RmT4jN5fi4KR6ziL42KrGK3AKqqTsR5ifJw > > https://dl-web.dropbox.com/get/Screenshots/Screenshot%202015-08-06%2018.37. > 16.png?_subject_uid=453014287&w=AABTWaKebxEgiyt6I7V_iUp0b_79Uo0aE42jeG_- > uRRAUA Andrei, I cannot access this urls, it looks like you copied the url directly, but for sharing you have to create a sharing link in dropbox... ;) Thanks.

https://www.dropbox.com/s/baitvfwk0m90gj3/Screenshot%202015-08-06%2018.31.13.png?dl=0 https://www.dropbox.com/s/tg59f321pe7ztt3/Screenshot%202015-08-06%2018.37.06.png?dl=0 https://www.dropbox.com/s/s2m556wbv9ezv7q/Screenshot%202015-08-06%2018.37.16.png?dl=0

(In reply to Andrei Oprea [:andreio] from comment #14) > https://www.dropbox.com/s/baitvfwk0m90gj3/Screenshot%202015-08-06%2018.31.13. > png?dl=0 > Height of the input area should 28px. Margin to both sides inside the input field is 10px so separate the icons. Use icons in the provided size. Text size should be 12 px for both placeholder and active text Check with visual specs always first. > https://www.dropbox.com/s/tg59f321pe7ztt3/Screenshot%202015-08-06%2018.37.06. > png?dl=0 > Highlight color is OK. The availability area there should not allow double line. > https://www.dropbox.com/s/s2m556wbv9ezv7q/Screenshot%202015-08-06%2018.37.16. > png?dl=0 How's the panel behaving? I think once the user signed in, it should have a fixed size. The illustrations for empty states do rely on this premise. It is Ok to have a shorter height for the FTU, but only in that case.

Comment on attachment 8645954 [details] [diff] [review] Bug 1183618 - Implement refreshed design for the contacts list Not sure if this needs another review or not. Mike's last comment was only about icons, and that is fixed. Better be safe! :)

https://www.dropbox.com/s/kcbytco1wzq9txu/Screenshot%202015-08-10%2013.51.16.png?dl=0 https://www.dropbox.com/s/7x0u4pfel1umjy0/Screenshot%202015-08-10%2013.51.04.png?dl=0 https://www.dropbox.com/s/11oxpsj6ob37jmk/Screenshot%202015-08-10%2013.50.52.png?dl=0

(In reply to Andrei Oprea [:andreio] from comment #18) > https://www.dropbox.com/s/kcbytco1wzq9txu/Screenshot%202015-08-10%2013.51.16. > png?dl=0 > OK > https://www.dropbox.com/s/7x0u4pfel1umjy0/Screenshot%202015-08-10%2013.51.04. > png?dl=0 > ok > https://www.dropbox.com/s/11oxpsj6ob37jmk/Screenshot%202015-08-10%2013.50.52. > png?dl=0 Can you please adjust the "MY contacts" title to be 11px, and line up with the thumbnails, everything in the list should have a 15px margin both left and right. This is a later adjustment: make the text in the first line of the contacts list names one pixel smaller, this is just to adjust hierarchies. Thanks!

Comment on attachment 8645954 [details] [diff] [review] Bug 1183618 - Implement refreshed design for the contacts list Review of attachment 8645954 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately this doesn't currently apply, so I haven't tested it. I did have a quick scan through and spotted a couple of issues. Can you fix it up and pass it to me again to give it a quick once-over? ::: browser/components/loop/content/css/contacts.css @@ +18,5 @@ > +} > + > +.contact-filter { > + margin: 0; > + padding-left: 34px; This won't work for rtl locales, since we're browser only, we can use -moz-padding-start instead. ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +85,5 @@ > # Contact Strings (Panel) > > ## LOCALIZATION NOTE(contacts_search_placeholder): This is the placeholder text for > ## the search field. > +contacts_search_placesholder=Type in contact name, email, phone number It seems the id name for the string was missed.

There might have been an issue due to the fact that I had a typo in the bug number. The new patch does not fix your comments above (but I will do it!).

Comment on attachment 8647362 [details] [diff] [review] Implement refreshed design for the contacts list Review of attachment 8647362 [details] [diff] [review]: ----------------------------------------------------------------- I'm liking the new layout. I don't think this is far off, just some small things to polish. ::: browser/components/loop/content/css/contacts.css @@ +23,5 @@ > +} > + > +.contact-filter { > + margin: 0; > + padding-left: 34px; This needs an rtl version @@ +29,5 @@ > + height: 28px; > + border: 0; > + border-bottom: 1px solid #ddd; > + background-image: url("../shared/img/icons-14x14.svg#magnifier"); > + background-position: 10px center; This needs an rtl version to make the magnifier appear on the right. @@ +410,5 @@ > background: #00A9DC; > color: #fff; > } > > +.button.primary:active { According to the specifications, this should be for hover as well. Currently neither the add contact nor import buttons have hover, but I think they should. Can you either fix that here or file a new bug please? ::: browser/components/loop/content/js/contacts.jsx @@ +435,5 @@ > + }); > + > + if (this._showFilter()) { > + let filter = this.state.filter.trim().toLocaleLowerCase(); > + if (filter) { toLocaleLowerCase is typically quite expensive, is it easy to pass the reduced filter into the filterContact function as a slight optimisation? @@ +449,5 @@ > + shownContacts.blocked = shownContacts.blocked > + .filter(this.filterContact); > + // Filter can return an empty array. > + if (!shownContacts.available.length) { > + shownContacts.available = null; Surely these should be shownContacts.blocked? Please also make sure there's a test covering this. @@ +607,5 @@ > return contact1._guid - contact2._guid; > }, > > + _renderFilterClearButton: function() { > + if (this.state.filter) { Can you generally change this to return null first? I think its what we generally do and it gets the "not doing anything" case out the way first, and reduces the indentation of the complex case. @@ +648,5 @@ > return ( > <div> > + {!this.state.filter ? <div className="contact-list-title"> > + {mozL10n.get("contact_list_title")} > + </div> : null} nit: I'd suggest indenting the last two lines by one more space each. @@ +664,5 @@ > </div> > ); > } > > + // If no contacts to show and filter is set then none match the search. Nit: please add a comma between "set" and "then", I think the reading will flow better. @@ +696,2 @@ > > + if (!this.state.filter) { Again, I'd recommend swapping this to the positive version. @@ +717,5 @@ > + return null; > + }, > + > + _renderGravatarPromoMessage: function() { > + if (!this.state.filter) { Can you swap this to the positive version, i.e. if (this.state.filter) { return null; } return <... @@ +730,5 @@ > + }, > + > + render: function() { > + let cx = React.addons.classSet; > + let showFilter = Object.getOwnPropertyNames(this.contacts).length >= You now don't need cx nor showFilter in this function. ::: browser/components/loop/content/shared/img/icons-14x14.svg @@ +79,5 @@ > <use id="volume" xlink:href="#volume-shape"/> > <use id="volume-active" xlink:href="#volume-shape"/> > <use id="volume-disabled" xlink:href="#volume-shape"/> > + <use id="clear" xlink:href="#clear-shape"/> > + <use id="magnifier" xlink:href="#magnifier-shape"/> Please add these two to the showcase ::: browser/components/loop/test/desktop-local/contacts_test.js @@ +352,5 @@ > + > + it("should not render the filter view unless MIN_CONTACTS_FOR_FILTERING", > + function() { > + var filterView = listView.getDOMNode() > + .querySelector(".contact-filter-container"); nit: 2-space indent please. ::: browser/components/loop/ui/ui-showcase.jsx @@ +772,5 @@ > notifications={notifications} > roomStore={roomStore} > selectedTab="contacts" /> > </Example> > + <Example dashed={true} style={{width: "332px"}} summary="Contact list tab (no search filter)"> Can you also add an example for no matching contacts, if that's easy? ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +85,5 @@ > # Contact Strings (Panel) > > ## LOCALIZATION NOTE(contacts_search_placeholder): This is the placeholder text for > ## the search field. > +contacts_search_placesholder=Type in contact name, email, phone number I'm concerned about this placeholder, its really quite long, and I bet L10n will have trouble fitting it in. We have got a bug to remove the phone number part which should help, but we either need to put a restriction in here and hope L10n keep to it, or find a way to cope with the display.

Comment on attachment 8649044 [details] [diff] [review] Implement refreshed design for the contacts list Review of attachment 8649044 [details] [diff] [review]: ----------------------------------------------------------------- r=me, this looks really great!! ::: browser/components/loop/content/js/contacts.jsx @@ +417,5 @@ > + * Filter a user by name, email or phone number. > + * Takes in an input to filter by and returns a filter function which > + * expects a contact. > + * > + * @returns {function} nit: uppercase 'F'. @@ +466,5 @@ > + * @returns {bool} > + */ > + _showFilter: function() { > + return Object.getOwnPropertyNames(this.contacts).length >= > + MIN_CONTACTS_FOR_FILTERING; Please rename this function to `_shouldShowFilter`, to make it clear that we're asking that question instead of really showing something called a 'filter'. Nit: please indent the next line properly with just two spaces. @@ +625,5 @@ > + <div className="contact-filter-container"> > + <input className="contact-filter" > + placeholder={mozL10n.get("contacts_search_placesholder")} > + valueLink={this.linkState("filter")} /> > + {this._renderFilterClearButton()} I'd prefer using a conditional here, but I won't block landing this patch on that change. ::: browser/components/loop/test/desktop-local/contacts_test.js @@ +353,5 @@ > + > + it("should not render the filter view unless MIN_CONTACTS_FOR_FILTERING", > + function() { > + var filterView = listView.getDOMNode() > + .querySelector(".contact-filter-container"); nit: just indent with two spaces, please. @@ +426,5 @@ > + > + it("should render the filter view for >= MIN_CONTACTS_FOR_FILTERING", > + function() { > + var filterView = listView.getDOMNode() > + .querySelector(".contact-filter-container"); nit: just two spaces, please. @@ +433,5 @@ > + }); > + > + it("should filter by name", function() { > + var input = listView.getDOMNode() > + .querySelector(".contact-filter-container input"); nit: just two spaces, please. @@ +436,5 @@ > + var input = listView.getDOMNode() > + .querySelector(".contact-filter-container input"); > + > + React.addons.TestUtils.Simulate.change(input, > + {target: {value: "Ally"}}); nit: just two spaces, please. Format the object literal as `{ target: { value: "Ally" } }` of make it spread multiple lines. @@ +444,5 @@ > + }); > + > + it("should filter by email", function() { > + var input = listView.getDOMNode() > + .querySelector(".contact-filter-container input"); nit: just two spaces, please. @@ +447,5 @@ > + var input = listView.getDOMNode() > + .querySelector(".contact-filter-container input"); > + > + React.addons.TestUtils.Simulate.change(input, > + {target: {value: "@hotmail"}}); nit: just two spaces, please. Format the object literal as `{ target: { value: "@hotmail" } }` of make it spread multiple lines. @@ +455,5 @@ > + }); > + > + it("should filter by phone number", function() { > + var input = listView.getDOMNode() > + .querySelector(".contact-filter-container input"); nit: just two spaces, please. @@ +458,5 @@ > + var input = listView.getDOMNode() > + .querySelector(".contact-filter-container input"); > + > + React.addons.TestUtils.Simulate.change(input, > + {target: {value: "12345678"}}); nit: just two spaces, please. Format the object literal as `{ target: { value: "12345678" } }` of make it spread multiple lines. ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +142,5 @@ > no_conversations_message_heading=There are no conversations yet > ## LOCALIZATION NOTE(no_converastions_start_message): Subheading inviting the > ## user to start a new conversation. > no_conversations_start_message=start a new conversation! > +## LOCALIZATION NOTE(no_search_results_message_heading): Title show when search nit: 'Title to show (...)'

I filed bug 1196251 to fix the string changed without a new ID, which is the most important thing to fix. Please keep an eye out when reviewing. Having said that, the design is just bad in terms of localizability. The sentence is split in two parts, with the first one being visually more relevant. I hope you understand that nobody guarantees that a language will have the most important concept ("no matching results") in the first part of a natural sounding sentence. Design is making assumptions on the grammar, that's just wrong, so please let's not do that.

Verified fixed using latest Nightly 43.0a1 and 44.0a1, across platforms [1]. [1] Windows 10 32-bit, Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit