1183636 - Implement the refreshed design for the panel footer

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

As the acceptance criteria in bug 1179193 states:

 - Tab footer refresh

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

Comment 1

[Andrei Oprea [:andreio]]

The cog/settings icon we currently have https://www.dropbox.com/sh/0f7imuid7f99lse/AAB22ORt-Kyaet8vh6l5b2Ofa/ico_settings.svg?dl=0 is broken, as it relies on FontAwesome.

Comment 2

[Victoria Gerchinhoren [:vicky]]

(In reply to Andrei Oprea [:andreio] from comment #1)
> The cog/settings icon we currently have
> https://www.dropbox.com/sh/0f7imuid7f99lse/AAB22ORt-Kyaet8vh6l5b2Ofa/
> ico_settings.svg?dl=0 is broken, as it relies on FontAwesome.

Fixed ;)!

Comment 3

[Andrei Oprea [:andreio]]

Attachment: Implement refreshed design for Loop panel footer

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8639634 [details] [diff] [review]
Implement refreshed design for Loop panel footer

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

::: browser/components/loop/content/js/panel.jsx
@@ +173,5 @@
>                <span>{mozL10n.get("display_name_available_status")}</span>
>              </li>
>              <li className="dropdown-menu-item dnd-make-unavailable"
>                  onClick={this.changeAvailability("do-not-disturb")}>
> +              <img src="loop/shared/img/icons-16x16.svg#status-unavailable" />

Please don't use <img> element, but classNames and pseudo-element instead.

@@ +632,5 @@
> +   * User profile prop can be either an object or null as per mozLoopAPI
> +   * and there is no way to express this with React 0.12.2
> +   */
> +  function userProfileValidator(props, propName, componentName) {
> +    if (typeof props[propName] !== "object") {

But you're not checking for `null` here??

::: browser/components/loop/content/shared/img/icons-10x10.svg
@@ +28,5 @@
>      <path id="dropdown-shape" fill-rule="evenodd" d="M9,3L4.984,7L1,3H9z"/>
>      <polygon id="expand-shape" points="10,0 4.838,0 6.506,1.669 0,8.175 1.825,10 8.331,3.494 10,5.162"/>
>      <path id="edit-shape" d="M5.493,1.762l2.745,2.745L2.745,10H0V7.255L5.493,1.762z M2.397,9.155l0.601-0.601L1.446,7.002L0.845,7.603 V8.31H1.69v0.845H2.397z M5.849,3.028c0-0.096-0.048-0.144-0.146-0.144c-0.044,0-0.081,0.015-0.112,0.046L2.014,6.508 C1.983,6.538,1.968,6.577,1.968,6.619c0,0.098,0.048,0.146,0.144,0.146c0.044,0,0.081-0.015,0.112-0.046l3.579-3.577 C5.834,3.111,5.849,3.073,5.849,3.028z M10,2.395c0,0.233-0.081,0.431-0.245,0.595L8.66,4.085L5.915,1.34L7.01,0.25 C7.168,0.083,7.366,0,7.605,0c0.233,0,0.433,0.083,0.601,0.25l1.55,1.544C9.919,1.966,10,2.166,10,2.395z"/>
>      <rect id="minimize-shape" y="3.6" width="10" height="2.8"/>
> +    <path id="cog" d="M122.285722,122.071424 C122.285722,122.936938 121.579806,123.642854 120.714291,123.642854 C119.848777,123.642854 119.142861,122.936938 119.142861,122.071424 C119.142861,121.205909 119.848777,120.499993 120.714291,120.499993 C121.579806,120.499993 122.285722,121.205909 122.285722,122.071424 L122.285722,122.071424 Z M125.428583,121.402338 C125.428583,121.297985 125.354922,121.199771 125.250569,121.181356 L124.127242,121.009481 C124.065858,120.806913 123.97992,120.604346 123.875567,120.407917 C124.084273,120.119413 124.311394,119.849323 124.520099,119.566957 C124.550791,119.523988 124.569207,119.481019 124.569207,119.425773 C124.569207,119.376666 124.55693,119.327559 124.526238,119.290729 C124.268425,118.928563 123.838737,118.547982 123.513402,118.247201 C123.470433,118.21037 123.415187,118.185817 123.359942,118.185817 C123.304696,118.185817 123.249451,118.204232 123.21262,118.241062 L122.340967,118.897871 C122.162954,118.805795 121.978802,118.732134 121.788511,118.67075 L121.616636,117.541285 C121.604359,117.436932 121.506145,117.357133 121.395654,117.357133 L120.032929,117.357133 C119.922438,117.357133 119.8365,117.430793 119.811947,117.529008 C119.713732,117.897312 119.676902,118.296308 119.633933,118.67075 C119.443642,118.732134 119.253352,118.811933 119.075338,118.904009 L118.228239,118.247201 C118.179132,118.21037 118.123886,118.185817 118.068641,118.185817 C117.859935,118.185817 117.031251,119.082023 116.88393,119.28459 C116.853238,119.327559 116.828684,119.370528 116.828684,119.425773 C116.828684,119.481019 116.853238,119.530126 116.890068,119.573095 C117.117189,119.849323 117.338171,120.125551 117.546877,120.420194 C117.448662,120.604346 117.368863,120.788498 117.307479,120.984927 L116.165737,121.156802 C116.073661,121.175217 116,121.285709 116,121.377785 L116,122.74051 C116,122.844862 116.073661,122.943077 116.178014,122.961492 L117.301341,123.127229 C117.362725,123.335934 117.448662,123.538502 117.553015,123.73493 C117.34431,124.023435 117.117189,124.293525 116.908483,124.575891 C116.877791,124.61886 116.859376,124.661829 116.859376,124.717074 C116.859376,124.766182 116.871653,124.815289 116.902345,124.858258 C117.160158,125.214285 117.589846,125.594866 117.915181,125.889509 C117.95815,125.932478 118.013395,125.957031 118.068641,125.957031 C118.123886,125.957031 118.179132,125.938616 118.222101,125.901786 L119.087615,125.244977 C119.265629,125.337053 119.449781,125.410714 119.640071,125.472098 L119.811947,126.601563 C119.824223,126.705916 119.922438,126.785715 120.032929,126.785715 L121.395654,126.785715 C121.506145,126.785715 121.592083,126.712054 121.616636,126.61384 C121.714851,126.245536 121.751681,125.84654 121.79465,125.472098 C121.98494,125.410714 122.175231,125.330914 122.353244,125.238838 L123.200343,125.901786 C123.249451,125.932478 123.304696,125.957031 123.359942,125.957031 C123.568647,125.957031 124.397331,125.054686 124.544653,124.858258 C124.581483,124.815289 124.599899,124.77232 124.599899,124.717074 C124.599899,124.661829 124.575345,124.606583 124.538515,124.563614 C124.311394,124.287386 124.090411,124.017297 123.881706,123.716515 C123.97992,123.538502 124.053581,123.35435 124.121103,123.157921 L125.256707,122.986046 C125.354922,122.96763 125.428583,122.857139 125.428583,122.765063 L125.428583,121.402338 Z" transform="translate(-116 -117)" fill="#666" fill-rule="evenodd"/>

Usually the ID of a path/ group inside the defines section is like 'cog-shape'. Can you change that? (Also for the 16x16px ones!)

::: browser/components/loop/content/shared/img/svg/glyph-settings-16x16.svg
@@ -1,5 @@
> -<?xml version="1.0" encoding="utf-8"?>
> -<!-- This Source Code Form is subject to the terms of the Mozilla Public
> -   - License, v. 2.0. If a copy of the MPL was not distributed with this
> -   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> -<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16">

I assume then, that you've removed the `.settings-menu .icon-settings` in one of your other patches?

::: browser/components/loop/standalone/server.js
@@ +89,5 @@
>  // appears to be to be to closely filter the url and match appropriately.
>  function serveIndex(req, res) {
>    "use strict";
>  
> +  return res.sendfile(path.join(__dirname, "content", "index.html"));

Good change! Poor Windows users :(

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +72,5 @@
>        logOutFromFxA: sandbox.stub(),
>        notifyUITour: sandbox.stub(),
>        openURL: sandbox.stub(),
> +      getSelectedTabMetadata: sandbox.stub(),
> +      userProfile: {}

Can you add a test for it being `null`, too?

Comment 5

[Andrei Oprea [:andreio]]

What is the hover state for the Available/Do not disturb button in the PanelUI footer? Right now it is a gray background with a slightly darker border. This used to match the hover state of the menu items but that is no longer true.

Comment 6

[Andrei Oprea [:andreio]]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Comment on attachment 8639634 [details] [diff] [review]
> Implement refreshed design for Loop panel footer
> 
> Review of attachment 8639634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/loop/content/shared/img/svg/glyph-settings-16x16.svg
> @@ -1,5 @@
> > -<?xml version="1.0" encoding="utf-8"?>
> > -<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > -   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > -   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > -<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16">
> 
> I assume then, that you've removed the `.settings-menu .icon-settings` in
> one of your other patches?
> 

I missed that. Thanks!

Comment 7

[Andrei Oprea [:andreio]]

Attachment: Implement refreshed design for Loop panel footer

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8640266 [details] [diff] [review]
Implement refreshed design for Loop panel footer

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

Super, almost there!

::: browser/components/loop/content/css/panel.css
@@ +686,5 @@
>  /* Settings (gear) menu */
>  
>  .button-settings {
> +  width: .8rem;
> +  height: .8rem;

If the icon determines the dimensions of the button, why not specify a width/ height in pixels too?

We're not going to bend-over backwards to try and use rem's everywhere... just where it makes sense to scale things proportionally to the font-size of the root element.

::: browser/components/loop/content/js/panel.jsx
@@ +155,2 @@
>        var availabilityText = this.state.doNotDisturb ?
> +                               mozL10n.get("display_name_dnd_status") :

nit: ah, since you're touching this, can you just indent this properly?

@@ +158,5 @@
>  
>        return (
>          <div className="dropdown">
> +          <p className="dnd-status">
> +            <span className={statusIcon} onClick={this.toggleDropdownMenu}

Nice change, can you put `onClick` on its own line too?

@@ +169,1 @@
>                  onClick={this.changeAvailability("available")}>

<3

@@ +454,5 @@
>        this.closeWindow();
>      },
>  
> +    /*
> +     * Limit email to 24 chars and add ellipsis.

Please see the _truncate function in roomViews.jsx.

Can you do the following:
1) Move the function to utils.js
2) add your RTL support to it

...and end up with moar awesomeness in the tree?

@@ +629,5 @@
> +   * and there is no way to express this with React 0.12.2
> +   */
> +  function userProfileValidator(props, propName, componentName) {
> +    if (Object.prototype.toString.call(props[propName]) !== "[object Object]"
> +        && !_.isNull(props[propName])) {

We don't put operands at the start of an expression.

@@ +630,5 @@
> +   */
> +  function userProfileValidator(props, propName, componentName) {
> +    if (Object.prototype.toString.call(props[propName]) !== "[object Object]"
> +        && !_.isNull(props[propName])) {
> +      return new Error("Required prop `" + propName +

nit: please don't use backticks, just single quotes.

@@ +645,5 @@
>      propTypes: {
>        dispatcher: React.PropTypes.instanceOf(loop.Dispatcher).isRequired,
>        mozLoop: React.PropTypes.object.isRequired,
>        store: React.PropTypes.instanceOf(loop.store.RoomStore).isRequired,
> +      // for room creation

nit: please fix the comment to be more descriptive while you're here :)

::: browser/components/loop/content/shared/css/common.css
@@ +65,5 @@
>  
> +/* Force view in the showcase */
> +.force-menu-show * {
> +  display: inline-block !important;
> +}

well, if this is only for the showcase, can you put the rule in ui-showcase.css?

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +141,2 @@
>          var availableMenuOption = view.getDOMNode()
> +                                        .querySelector(".status-available");

nit: if you start aligning things like this, please align on the dots.

@@ +296,5 @@
> +      it("should throw an error when user profile is different from {} or null",
> +         function() {
> +          var warnstub = sandbox.stub(console, "warn");
> +          var view = TestUtils.renderIntoDocument(
> +                       React.createElement(loop.panel.AccountLink, {

nit: indentation.

@@ +309,5 @@
> +      it("should throw an error when user profile is different from {} or null",
> +         function() {
> +          var warnstub = sandbox.stub(console, "warn");
> +          var view = TestUtils.renderIntoDocument(
> +                       React.createElement(loop.panel.AccountLink, {

nit: indentation.

@@ +347,5 @@
> +           function() {
> +             var view = mountTestComponent();
> +
> +             expect(view.getDOMNode().querySelectorAll(".icon-signout"))
> +             .to.have.length.of(0);

nit: indentation.

@@ +349,5 @@
> +
> +             expect(view.getDOMNode().querySelectorAll(".icon-signout"))
> +             .to.have.length.of(0);
> +             expect(view.getDOMNode().querySelectorAll(".icon-signin"))
> +             .to.have.length.of(1);

nit: indentation.

@@ +357,5 @@
> +           function() {
> +             var view = mountTestComponent();
> +
> +             expect(view.getDOMNode().querySelectorAll(".icon-account"))
> +             .to.have.length.of(0);

nit: indentation.

@@ +367,5 @@
>  
> +          TestUtils.Simulate.click(
> +            view.getDOMNode().querySelector(".icon-signin"));
> +
> +            sinon.assert.calledOnce(navigator.mozLoop.logInToFxA);

nit: whoops. indentation.

Comment 9

[Mike de Boer [:mikedeboer]]

Also, when you think you've got the UI perfected, can you post a (few) screenshot(s) and request ui-r from Vicky?
Thanks!

Comment 10

[Andrei Oprea [:andreio]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Comment on attachment 8640266 [details] [diff] [review]
> Implement refreshed design for Loop panel footer
> 
> Review of attachment 8640266 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +630,5 @@
> > +   */
> > +  function userProfileValidator(props, propName, componentName) {
> > +    if (Object.prototype.toString.call(props[propName]) !== "[object Object]"
> > +        && !_.isNull(props[propName])) {
> > +      return new Error("Required prop `" + propName +
> 
> nit: please don't use backticks, just single quotes.

That's how React warnings show up in the console. Didn't want our validator to output different messages.

Comment 11

[Andrei Oprea [:andreio]]

Attachment: Implement refreshed design for Loop panel footer

Comment 12

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-29 at 5.48.24 PM.png

Comment 13

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-29 at 5.48.39 PM.png

Comment 14

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-29 at 5.48.44 PM.png

Comment 15

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-29 at 5.48.52 PM.png

Comment 16

[Andrei Oprea [:andreio]]

Attachment: Screen Shot 2015-07-29 at 5.48.56 PM.png

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8640827 [details] [diff] [review]
Implement refreshed design for Loop panel footer

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

Great! With the `truncate` function and the few nits fixed, r=me!

::: browser/components/loop/content/shared/js/utils.js
@@ +750,5 @@
> +    maxLen = maxLen || 72;
> +
> +    if (str.length > maxLen) {
> +      var substring = str.substr(0, maxLen);
> +      var direction = document.mozL10n ? mozL10n.getDirection() :

This check is a bit silly... mozL10n is _always_ available in utils.js... this allows you to shorten the function body quite a bit.

@@ +753,5 @@
> +      var substring = str.substr(0, maxLen);
> +      var direction = document.mozL10n ? mozL10n.getDirection() :
> +                      mozL10n.language.direction;
> +      if (direction === "rtl") {
> +        return "..." + substring;

This is not a unicode ellipsis char, which is why I asked to copy the _truncate function from roomViews.jsx and add the RTL logic in here.
Please do this.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +289,5 @@
> +        };
> +        var view = createTestPanelView();
> +        var node = view.getDOMNode().querySelector(".user-identity");
> +
> +        expect(node.textContent).to.eql("reallyreallylongtext@exa...");

...so this will become an actual ellipsis.

@@ +368,4 @@
>            var view = mountTestComponent();
>  
> +          TestUtils.Simulate.click(view.getDOMNode()
> +                                   .querySelector(".icon-signin"));

nit: indentation.

@@ +369,5 @@
>  
> +          TestUtils.Simulate.click(view.getDOMNode()
> +                                   .querySelector(".icon-signin"));
> +
> +            sinon.assert.calledOnce(navigator.mozLoop.logInToFxA);

nit: indentation.

@@ +402,3 @@
>  
> +           TestUtils.Simulate.click(view.getDOMNode()
> +                                    .querySelector(".icon-account"));

nit: indentation.

@@ +410,5 @@
>          navigator.mozLoop.userProfile = {email: "[e-mail]"};
>          var view = mountTestComponent();
>  
> +        TestUtils.Simulate.click(view.getDOMNode()
> +                                 .querySelector(".icon-signout"));

nit: indentation.

Comment 18

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8640836 [details]
Screen Shot 2015-07-29 at 5.48.56 PM.png

Can you please use #666666 for the text? And also equal padding on both sides, I think 15px on each side would be OK. Thanks.

Comment 19

[Andrei Oprea [:andreio]]

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

Comment 20

[Victoria Gerchinhoren [:vicky]]

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

PLease. put the "or" between Sing in and Sign Up in gray text and regular style.
Besides that, it looks ok. Don't need to review again.

Comment 21

[Andrei Oprea [:andreio]]

What is the correct way to break the "Sign in or Sign up" string so that Sign in and Sign up can different links to 2 different pages (see attachment for UI).
The two suggestions are:
1.  <a>string1</a> string2 <a>string3</a>
2. str1 <a>str2</a> str3 <a>str4</a> str5
Right now I went ahead with option 1 but let me know if the second one is needed.

Comment 22

[Axel Hecht [:Pike]]

You'll need the second, and I'd advise against breaking them up.

Can't really be constructive here, cause hello uses some l10n infra that's unknown/undefined to my team.

A bug like this might be a good opportunity to upgrade to something supported.

Comment 23

[Andrei Oprea [:andreio]]

This particular code is in the browser chrome and is actually using the normal Firefox JS localization infrastructure. Does that affect your answer?

Comment 24

[Francesco Lodolo [:flod]]

In the code I see references to 
> mozL10n.get("panel_footer_signin_or_signup_link")

So I assume it's still using the patched version of l10n.js like the rest of Loop and not the standard localization process? A supported version of l10n.js would have better approaches to these issues, with what we have I think only this is available:

signin_link=Sign in
signup_link=Sign up
signin_or_signup_message=#1 or #2

And a clear localization message explaining what's going to happen in the message string.

What I don't understand is if we're going to have 2 different URLs, or this is just an aesthetic change. If it's the latter, I would simply avoid it.

Comment 25

[Andrei Oprea [:andreio]]

(In reply to Francesco Lodolo [:flod] from comment #24)
> In the code I see references to 
> > mozL10n.get("panel_footer_signin_or_signup_link")
> 
> So I assume it's still using the patched version of l10n.js like the rest of
> Loop and not the standard localization process? A supported version of
> l10n.js would have better approaches to these issues, with what we have I
> think only this is available:
> 
> signin_link=Sign in
> signup_link=Sign up
> signin_or_signup_message=#1 or #2
> 
> And a clear localization message explaining what's going to happen in the
> message string.
> 
> What I don't understand is if we're going to have 2 different URLs, or this
> is just an aesthetic change. If it's the latter, I would simply avoid it.

Yes there will be 2 different links.

Comment 26

[Andrei Oprea [:andreio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d9167d05a5d

Comment 27

[Andrei Oprea [:andreio]]

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

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3b1c7714b26

Comment 29

[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 the panel footer matches the implementation here. I did found an issue, bug 1196242.

Comment 30

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

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #30)
> [Feature/regressing bug #]: FF Hello Visual Refresh, backout patch 2 of 5

Make that 4 of 5...

Comment 32

[Sylvestre Ledru [:Sylvestre]]

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

managed in bug 1203454