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.

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

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

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 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?)

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

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

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.

Updated the distance between menu background shape and settings button

Updated screenshot with your comments

Comment on attachment 8651671 [details] 1184917-1.PNG Thanks, Marina!

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?

Would you mind reviewing the patch again?

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?

Final patch

Is there a Try link handy for this?

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.