Closed Bug 1184917 Opened 4 years ago Closed 4 years ago

Implement the refreshed design for 'Edit' conversation toolbar button

Categories

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

defect
Points:
5

Tracking

(firefox43 verified)

VERIFIED FIXED
mozilla43
Iteration:
43.2 - Sep 7
Tracking Status
firefox43 --- verified

People

(Reporter: mikedeboer, Assigned: mai)

References

Details

(Whiteboard: [visual refresh])

Attachments

(2 files, 6 obsolete files)

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.
Flags: qe-verify+
Flags: firefox-backlog+
Rank: 19
Rank: 19 → 17
Assignee: nobody → marina.rodrigueziglesias
Hi Mark,
would you mind reviewing this patch?
Best regards
Attachment #8647481 - Flags: review?(standard8)
Attached image conversationToolbarEditMenu.PNG (obsolete) —
Hi Vicky,
would you mind reviewing this screenshot?
Best regards
Attachment #8647483 - Flags: ui-review?(vpg)
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.
Attachment #8647481 - Flags: review?(standard8) → review-
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?)
Attachment #8647483 - Flags: ui-review?(vpg) → ui-review?
Iteration: --- → 43.1 - Aug 24
Hi Mike, 
would you mind to review the patch?
Regards
Attachment #8647481 - Attachment is obsolete: true
Attachment #8650894 - Flags: review?(mdeboer)
Attached image 1184917.PNG (obsolete) —
Hi Pau,
would you mind reviewing the attachment?
Best regards
Attachment #8647483 - Attachment is obsolete: true
Attachment #8647483 - Flags: ui-review?
Attachment #8650896 - Flags: ui-review?(b.pmm)
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.
Attachment #8650896 - Flags: ui-review?(b.pmm) → ui-review-
Updated the distance between menu background shape and settings button
Attachment #8650894 - Attachment is obsolete: true
Attachment #8650894 - Flags: review?(mdeboer)
Attachment #8651648 - Flags: review?(mdeboer)
Attached image 1184917-1.PNG
Updated screenshot with your comments
Attachment #8650896 - Attachment is obsolete: true
Attachment #8651671 - Flags: ui-review?(b.pmm)
Comment on attachment 8651671 [details]
1184917-1.PNG

Thanks, Marina!
Attachment #8651671 - Flags: ui-review?(b.pmm) → ui-review+
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7
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?
Attachment #8651648 - Flags: review?(mdeboer) → feedback+
Would you mind reviewing the patch again?
Attachment #8651648 - Attachment is obsolete: true
Attachment #8652223 - Flags: review?(mdeboer)
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?
Attachment #8652223 - Flags: review?(mdeboer) → review+
Final patch
Attachment #8652223 - Attachment is obsolete: true
Attachment #8652373 - Flags: review+
Keywords: checkin-needed
Is there a Try link handy for this?
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c1d5f91e676e
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
QA Contact: bogdan.maris
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.