1184917 - Implement the refreshed design for 'Edit' conversation toolbar button

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

To meet the acceptance criteria as stated in bug 1179164, we should:

 - Change the icon of the button from a pencil to a cog.
 - Implement a context menu that will be anchored to the cog button, containing the following items:
    - 'Add Conversation Details' or 'Edit Conversation Details', depending whether there's no context attached yet to the conversation.
    - 'Submit Feedback' - direct link to open the NPS form.
    - 'Help' - direct link to open the help page(s), the same as in the panel.

Note: the menu looks and behaves just like the Settings menu in the Loop panel.

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

Comment 1

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Hi Mark,
would you mind reviewing this patch?
Best regards

Comment 2

[Marina Rodríguez [:mai]]

Attachment: conversationToolbarEditMenu.PNG

Hi Vicky,
would you mind reviewing this screenshot?
Best regards

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8647481 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

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

This is looking good. There's a few comments here to address. Please also add tests for the new SettingsControlButton, and make sure all tests pass (e.g. eslint etc).

For getting you started on the SettingsControlButton, you should be able to move the "Edit Context" section from roomViews_test.js into a new SettingsControlButton section in views.jsx and adapt & extend. Here's the section I'm on about moving: http://hg.mozilla.org/mozilla-central/annotate/7649ffe28b67/browser/components/loop/test/desktop-local/roomViews_test.js#l601

::: browser/components/loop/content/shared/css/conversation.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +html {
> +  font-size: 10px;
> +  font-family: menu;

I think this addition has messed up the screen share menu font size. I think we want it, but we'll need an additional fix there.

@@ +414,5 @@
> +  right: 14px;
> +}
> +
> +html[dir="rtl"] .settings-menu.dropdown-menu {
> + left: 14px;

nit: need 2 space indent.

::: browser/components/loop/content/shared/img/icons-10x10.svg
@@ +52,5 @@
>    <use id="minimize" xlink:href="#minimize-shape"/>
>    <use id="minimize-active" xlink:href="#minimize-shape"/>
>    <use id="minimize-disabled" xlink:href="#minimize-shape"/>
> +  <use id="settings-cog-grey" xlink:href="#cog-shape"/>
> +  <use id="settings-cog-white" xlink:href="#cog-shape"/>

Please update the ui-showcase with the new shapes.

::: browser/components/loop/content/shared/js/views.jsx
@@ +189,5 @@
> +      onEditClick: React.PropTypes.func,
> +      edit: React.PropTypes.object
> +    },
> +
> +    mixins: [sharedMixins.DropdownMenuMixin(), sharedMixins.WindowCloseMixin],

We shouldn't need the WindowCloseMixin here, but please add the React.addons.PureRenderMixin and split the mixins onto separate lines.

@@ +191,5 @@
> +    },
> +
> +    mixins: [sharedMixins.DropdownMenuMixin(), sharedMixins.WindowCloseMixin],
> +
> +    handleClick: function() {

Please add jsdocs for all the functions (apart from the react component ones, like render)

@@ +202,5 @@
> +      }
> +    },
> +
> +    handleHelpEntry: function(event) {
> +      event.preventDefault();

Lets be consistent and use preventDefault in all the handle functions.

@@ +214,5 @@
> +    },
> +
> +    render: function() {
> +      var cx = React.addons.classSet;
> +      var editEntryClasses = cx({"dropdown-menu-item": true, "entry-settings-edit": true, "hide": !this.props.edit.visible});

Nit: please split this onto multiple lines.

@@ +230,5 @@
> +
> +          <ul className={settingsDropdownMenuClasses} ref="menu">
> +            <li className={editEntryClasses}
> +              onClick={this.handleToggleEdit} scope="local" type="edit">
> +              {mozL10n.get(this.props.edit.enabled ? "context_edit_tooltip" : "context_hide_tooltip")}

As these aren't tooltips, please can you change their names to:

conversation_settings_menu_edit_context
conversation_settings_menu_hide_context

and update in loop.properties as well.

This will be clearer for L10n.

I think the other two are fine as they are buttons/menus already (tooltips may be handled differently by some locales, so we should be careful there).

@@ +323,5 @@
>                                        state={this.props.screenShare.state}
>                                        visible={this.props.screenShare.visible} />
>            </li>
>            <li className="conversation-toolbar-btn-box btn-edit-entry">
> +            <SettingsControlButton edit={this.props.edit}

This is currently displayed everywhere. Whilst I think displaying it on the direct calls as well as conversations is fine. I don't think it wants to be displayed on the standalone - the feedback survey isn't wanted there, and there will be a separate button for help.

Hence, I think you want another prop for ConversationToolbar, e.g. displaySettings, to display this button or not.

Comment 4

[Victoria Gerchinhoren [:vicky]]

Comment on attachment 8647483 [details]
conversationToolbarEditMenu.PNG

I think you might have to evolve the UI more and then have a greater UI review, because having elements in a different position and the UI so rough, it is difficult to review. Is this menu sharing style with other contextual menus of the UI (ex. panel UI settings?)

Comment 5

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Hi Mike, 
would you mind to review the patch?
Regards

Comment 6

[Marina Rodríguez [:mai]]

Attachment: 1184917.PNG

Hi Pau,
would you mind reviewing the attachment?
Best regards

Comment 7

[Pau Masiá [:Pau]]

Comment on attachment 8650896 [details]
1184917.PNG

Settings button should be in "click" state. Also, the gap between menu background shape and settings button should be 5px. There should also be 3 icons, 1 per option.

Comment 8

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Updated the distance between menu background shape and settings button

Comment 9

[Marina Rodríguez [:mai]]

Attachment: 1184917-1.PNG

Updated screenshot with your comments

Comment 10

[Pau Masiá [:Pau]]

Comment on attachment 8651671 [details]
1184917-1.PNG

Thanks, Marina!

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8651648 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

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

::: browser/components/loop/content/js/conversationViews.jsx
@@ +703,5 @@
>              <loop.shared.views.ConversationToolbar
>                audio={this.props.audio}
>                dispatcher={this.props.dispatcher}
>                edit={{ visible: false, enabled: false }}
> +              enableSettings={true}

Can you make this an array of objects:

```js
// 'visible' and 'enabled' are true by default.
var settingsMenuItems = [
  {
    id: "edit",
    visible: false,
    enabled: false
  },
  { id: "feedback" }, { id: "help" }
];
```

Please remove the `edit` attribute (because it now resides inside the settings menu) and rename `enabledSettings` to `settingsMenuItems`.

You'll have to make some changes to `SettingsControlButton` too.

::: browser/components/loop/content/shared/js/views.jsx
@@ +187,5 @@
> +    propTypes: {
> +      contextEdition: React.PropTypes.object,
> +      mozLoop: React.PropTypes.object,
> +      onEditClick: React.PropTypes.func,
> +      visible: React.PropTypes.bool

No required props??

::: browser/components/loop/test/shared/views_test.js
@@ +253,5 @@
>    });
>  
> +  describe("SettingsControlButton", function() {
> +    var fakeMozLoop,
> +        support_url = "https://support.com",

nit: one `var` declaration per line, please.

@@ +260,5 @@
> +    beforeEach(function() {
> +      fakeMozLoop = {
> +        openURL: sandbox.stub(),
> +        getStrings: function() {
> +          return JSON.stringify({textContent: "fakeText"});

nit: please format this object literal correctly.

@@ +277,5 @@
> +      };
> +    });
> +
> +    it("should render a visible button", function() {
> +      var comp = TestUtils.renderIntoDocument(

If you write a function `mountTestComponents` that mounts the component and sets props as you need them, it'd reduce the amount of boilerplate code needed here.
See https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/test/desktop-local/feedbackViews_test.js#28 for an example.

@@ -456,5 @@
>            sdk: fakeSDK,
>            model: model,
>            video: {enabled: true}
>          });
> -

nit: this change is not necessary, right?

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +361,5 @@
>  context_save_label2=Save
>  context_link_modified=This link was modified.
>  context_learn_more_link_label=Learn more.
> +conversation_settings_menu_edit_context=Edit Context
> +conversation_settings_menu_hide_context=Hide Context

When you don't change the strings, there's no real need to change the IDs. Can you revert this change?

Comment 12

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Would you mind reviewing the patch again?

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8652223 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

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

r=me with comments addressed.

::: browser/components/loop/content/js/roomViews.jsx
@@ +728,5 @@
> +            {
> +              id: "edit",
> +              enabled: !this.state.showEditContext,
> +              visible: this.state.contextEnabled,
> +              onEditClick: this.handleEditContextClick

Please rename this to `onClick`

::: browser/components/loop/content/shared/js/views.jsx
@@ +185,5 @@
> +   */
> +  var SettingsControlButton = React.createClass({
> +    propTypes: {
> +      mozLoop: React.PropTypes.object,
> +      settingsMenuItems: React.PropTypes.array

I think it's OK to call 'em just `menuItems` here...

@@ +193,5 @@
> +      sharedMixins.DropdownMenuMixin(),
> +      React.addons.PureRenderMixin
> +    ],
> +
> +    /*

Can you make all these comments into proper docblock comments?

@@ +221,5 @@
> +      event.preventDefault();
> +      var helloSupportUrl = this.props.mozLoop.getLoopPref("support_url");
> +      this.props.mozLoop.openURL(helloSupportUrl);
> +    },
> +    /*

nit: missing newline.

@@ +255,5 @@
> +              "hide": !menuItem.visible
> +            }),
> +            handler: this.getHandleToggleEdit(menuItem),
> +            label: mozL10n.get(menuItem.enabled ?
> +                "conversation_settings_menu_edit_context" :

nit: indentation.

@@ +275,5 @@
> +      if (!itemInfo) {
> +        return null;
> +      }
> +      return (
> +        <li className={itemInfo.cssClasses} key={menuItem.id} onClick={itemInfo.handler} scope={itemInfo.scope || ""} type={itemInfo.type || ""} >

nit: please format the attribute correctly on separate lines.

@@ +282,5 @@
> +        );
> +    },
> +
> +    render: function() {
> +      if (!this.props.settingsMenuItems || this.props.settingsMenuItems.length === 0) {

`if (!this.props.menuItems || !this.props.menuItems.length) {`

@@ +307,5 @@
> +             title={mozL10n.get("settings_menu_button_tooltip")} />
> +
> +          <ul className={settingsDropdownMenuClasses} ref="menu">
> +            {menuItemRows}
> +          </ul>

Can't you just return the button and have the <ul> nested inside the <button> element?
Just wondering if you tried it; I can understand why it wouldn't work.

@@ +325,2 @@
>          screenShare: {state: SCREEN_SHARE_STATES.INACTIVE, visible: false},
> +        settingsMenuItem: null,

This doesn't work when misspelled...

::: browser/components/loop/test/shared/views_test.js
@@ +361,5 @@
> +        {
> +          id: "edit",
> +          enabled: true,
> +          visible: true,
> +          onEditClick: function() { onEditClickCalled = true; }

Why not make this a stub?

Comment 14

[Marina Rodríguez [:mai]]

Attachment: 0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Final patch

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is there a Try link handy for this?

Comment 16

[Marina Rodríguez [:mai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6617ee0d6a30

Comment 17

[Pulsebot]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Bogdan Maris, Desktop QA]

Verified that the changes made to the Edit button are the same with the intended refresh across platforms (Windows 10 32-bit, Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit) using latest Nightly 43.0a1 and 44.0a1. 
One new issues found here on RTL builds, bug 1208025.