Implement the refreshed design for 'Edit' conversation toolbar button

VERIFIED FIXED in Firefox 43

Status

P1
normal
Rank:
17
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mai)

Tracking

unspecified
mozilla43
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(firefox43 verified)

Details

(Whiteboard: [visual refresh])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

3 years ago
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+

Updated

3 years ago
Rank: 19

Updated

3 years ago
Rank: 19 → 17
(Assignee)

Updated

3 years ago
Assignee: nobody → marina.rodrigueziglesias
(Assignee)

Comment 1

3 years ago
Created attachment 8647481 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Hi Mark,
would you mind reviewing this patch?
Best regards
Attachment #8647481 - Flags: review?(standard8)
(Assignee)

Comment 2

3 years ago
Created attachment 8647483 [details]
conversationToolbarEditMenu.PNG

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?

Updated

3 years ago
Iteration: --- → 43.1 - Aug 24
(Assignee)

Comment 5

3 years ago
Created attachment 8650894 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Hi Mike, 
would you mind to review the patch?
Regards
Attachment #8647481 - Attachment is obsolete: true
Attachment #8650894 - Flags: review?(mdeboer)
(Assignee)

Comment 6

3 years ago
Created attachment 8650896 [details]
1184917.PNG

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 7

3 years ago
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-
(Assignee)

Comment 8

3 years ago
Created attachment 8651648 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

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

Comment 9

3 years ago
Created attachment 8651671 [details]
1184917-1.PNG

Updated screenshot with your comments
Attachment #8650896 - Attachment is obsolete: true
Attachment #8651671 - Flags: ui-review?(b.pmm)

Comment 10

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

Thanks, Marina!
Attachment #8651671 - Flags: ui-review?(b.pmm) → ui-review+

Updated

3 years ago
Iteration: 43.1 - Aug 24 → 43.2 - Sep 7
(Reporter)

Comment 11

3 years ago
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+
(Assignee)

Comment 12

3 years ago
Created attachment 8652223 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Would you mind reviewing the patch again?
Attachment #8651648 - Attachment is obsolete: true
Attachment #8652223 - Flags: review?(mdeboer)
(Reporter)

Comment 13

3 years ago
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+
(Assignee)

Comment 14

3 years ago
Created attachment 8652373 [details] [diff] [review]
0001-Bug-1184917-Implement-the-refreshed-design-for-Edit-.patch

Final patch
Attachment #8652223 - Attachment is obsolete: true
Attachment #8652373 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Is there a Try link handy for this?
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c1d5f91e676e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
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
status-firefox43: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.