1183642 - Implement a context menu attached to a conversation/ room entry

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

As the acceptance criteria in bug 1179193 states:

 - Contextual menu for conversations (Video, Audio, Copy, E-mail, Share, Edit conversation detail, Delete Conversation)

Note: this does _not_ include the Audio and Share options, because they're out of scope for now.

The functionality and placement of this context menu is identical to what's implemented in the contacts list at the moment. It is advised to copy the functionality from there.

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

Comment 1

[Andrei Oprea [:andreio]]

Attachment: Implement context menu for Loop conversation entries

Comment 2

[Andrei Oprea [:andreio]]

Attached a patch for feedback? I'm interested in comments regarding the dropdown positioning script. Some notes are in the comments in the code. 
I've tested it to work with and without the NewRoomButton component displayed. (You will have to test this manually, remove the DOM node and set the correct height). But the position of the menu should update automatically (render up or down relative to where you clicked.)
There is also the matter of implementing it for RTL.

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8650905 [details] [diff] [review]
Implement context menu for Loop conversation entries

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

I applied the patch and I don't really know what I should look at.

I was hoping you'd be able to use & extend the code in DropdownMenuMixin in content/shared/js/mixins.js with the `boundingBoxSelector` as a starting point. See https://dxr.mozilla.org/mozilla-central/source/browser/components/loop/content/js/roomViews.jsx#150 as an example.

Comment 4

[Andrei Oprea [:andreio]]

I'm not sure what `Edit Conversation Details` in the context menu should do.

Comment 5

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #4)
> I'm not sure what `Edit Conversation Details` in the context menu should do.

I'm not sure of the exact details either. Given its new functionality I would propose splitting adding that item off to a different bug, and asking for clarification from UX.

I think there's enough remaining in the menu to make it worth having.

Comment 6

[Andrei Oprea [:andreio]]

UI Review?

GIF
https://www.dropbox.com/s/czq7mddciz1x8o6/ShinyGenuineLaughingthrush.gif?dl=0

In the showcase RTL mode fake strings
https://www.dropbox.com/s/dsqnwnnfpnxs1gs/Screenshot%202015-08-25%2015.13.39.png?dl=0

In FF panel with actual strings:
https://www.dropbox.com/s/np3za6ipzgrqcdk/Screenshot%202015-08-25%2015.13.19.png?dl=0

Comment 7

[Andrei Oprea [:andreio]]

Attachment: Implement context menu for Loop conversation entries

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8652703 [details] [diff] [review]
Implement context menu for Loop conversation entries

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

Hmm, this does look a lot better to me. Perhaps we can generalize the positioning code to move into the DropdownMenuMixin in a follow-up, tech-debt style.

UX is trying to make us move toward OS (XUL) style dropdowns, I don't think it's worth spending many more cycles on.

Please move forward with this approach! Thanks ;)

Comment 9

[Sevaan Franks [:sevaan]]

(In reply to Andrei Oprea [:andreio] from comment #6)
> UI Review?
> 
> GIF
> https://www.dropbox.com/s/czq7mddciz1x8o6/ShinyGenuineLaughingthrush.gif?dl=0
> 
> In the showcase RTL mode fake strings
> https://www.dropbox.com/s/dsqnwnnfpnxs1gs/Screenshot%202015-08-25%2015.13.39.
> png?dl=0
> 
> In FF panel with actual strings:
> https://www.dropbox.com/s/np3za6ipzgrqcdk/Screenshot%202015-08-25%2015.13.19.
> png?dl=0

Looking good, but can we make the top and bottom padding match the left and right? It seems a little large.

Comment 10

[Andrei Oprea [:andreio]]

Attachment: Implement context menu for Loop conversation entries

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8653206 [details] [diff] [review]
Implement context menu for Loop conversation entries

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

Only r- because of the amount of nits I found! Otherwise, this looks really good to me. Please attach a patch with the nits fixed and I'll review it asap. Thanks!

::: browser/components/loop/content/css/panel.css
@@ +490,5 @@
>  .room-list > .room-entry.room-active > h2:before {
>    background-image: url("../shared/img/icons-14x14.svg#hello-active");
>  }
>  
> +/* Room entry context */

s/context/dropdown/ ?

@@ +494,5 @@
> +/* Room entry context */
> +.room-entry-context-actions {
> +  display: none;
> +  border-radius: 30px;
> +  background: #56b397;

Did you get this color from a color picker or from the specs?

@@ +503,5 @@
> +  display: inline-block;
> +}
> +
> +.room-entry:hover .room-entry-context-actions:hover {
> +  background: #50e3c2;

And this one?

@@ +509,5 @@
> +
> +/* Room entry call button */
> +.room-entry-call-btn {
> +  border-top-left-radius: 30px;
> +  border-bottom-left-radius: 30px;

For a fully round radius effect, we usually use `radius: 10000px` (since 100% doesn't work as you'd expect).

::: browser/components/loop/content/js/panel.jsx
@@ -568,5 @@
> -      }.bind(this));
> -    },
> -
> -    handleMouseLeave: function(event) {
> -      this.setState({urlCopied: false});

nit: please format this object literal correctly.

@@ +551,5 @@
> +
> +      this.toggleDropdownMenu();
> +    },
> +
> +    /* When moving cursor away from the dropdown we need to close the menu. */

nit: Please change this to a proper docblock-style comment.

@@ +567,4 @@
>  
>        return (
> +        <div
> +          className={roomClasses}

nit: please put this on the same line as `<div `.

@@ +590,5 @@
>        );
>      }
>    });
>  
> +  /* This component renders the video icon call button and chevron button for

Can you use either docblock-style comments or `//`-style comments? This mix between styles is uncommon.

@@ +593,5 @@
>  
> +  /* This component renders the video icon call button and chevron button for
> +   * displaying contextual dropdown menu for conversation entries.
> +   * It also holds the dropdown menu. */
> +  var RoomEntryContextActions = React.createClass({

This name implies 'A set of actions for a room entry' for me, not a name of a component/ view. Can you change it?

@@ +601,5 @@
> +      handleClickEntry: React.PropTypes.func.isRequired,
> +      room: React.PropTypes.object.isRequired,
> +      showMenu: React.PropTypes.bool.isRequired,
> +      toggleDropdownMenu: React.PropTypes.func.isRequired,
> +      toggleMenu: React.PropTypes.func.isRequired

I understand what the difference between these two is, but could you make their names more distinctive so that future readers know exactly what each one does?
You could also attempt to unify them into one function and sprinkle some documenting comments on top.

@@ +644,5 @@
> +
> +    render: function() {
> +      return (
> +        <div
> +          className="room-entry-context-actions">

nit: please put this on the same line as `<div `.

@@ +666,5 @@
> +      );
> +    }
> +  });
> +
> +  /* Dropdown menu for each conversation entry.

nit: re-format comment.

@@ +680,5 @@
> +      handleEmailButtonClick: React.PropTypes.func.isRequired
> +    },
> +
> +    statics: {
> +      /* Look up the DOM hierarchy for a node matching `selector`.

nit: re-format comment.

@@ +685,5 @@
> +       * If it is not found return the parent node, this is a sane default so
> +       * that subsequent queries on the result do no fail.
> +       * Better choice than the alternative `document.querySelector(selector)`
> +       * because we ensure it works in the UI showcase as well. */
> +      findNode: function(node, selector) {

This looks generic enough to be moved to utils.js.

@@ +701,5 @@
> +        return node;
> +      }
> +    },
> +
> +    getInitialState: function () {

nit: `function() {` (drop the superfluous space).

@@ +707,5 @@
> +        openDirUp: false
> +      };
> +    },
> +
> +    componentDidMount: function () {

nit: same.

@@ +711,5 @@
> +    componentDidMount: function () {
> +      var menuNode = this.getDOMNode();
> +      var menuNodeRect = menuNode.getBoundingClientRect();
> +
> +      /* Get the parent element and make sure the menu does not overlow its

nit: please use `// ` comments.

@@ +716,5 @@
> +       * container. */
> +      var listNode = this.constructor.findNode(this.getDOMNode(), ".rooms");
> +      var listNodeRect = listNode.getBoundingClientRect();
> +
> +      /* Click offset to not display the menu right next to the area clicked. */

nit: same.

@@ +721,5 @@
> +      var offset = 10;
> +
> +      if (this.props.eventPosY + menuNodeRect.height >=
> +          listNodeRect.top + listNodeRect.height) {
> +        /* Position above click area. */

nit: same.

@@ +722,5 @@
> +
> +      if (this.props.eventPosY + menuNodeRect.height >=
> +          listNodeRect.top + listNodeRect.height) {
> +        /* Position above click area. */
> +        menuNode.style.top = this.props.eventPosY - menuNodeRect.height - listNodeRect.top - offset + "px";

nit: this seems to exceed 80chars...

@@ +724,5 @@
> +          listNodeRect.top + listNodeRect.height) {
> +        /* Position above click area. */
> +        menuNode.style.top = this.props.eventPosY - menuNodeRect.height - listNodeRect.top - offset + "px";
> +      } else {
> +        /* Position below click area. */

nit: please use `// ` comments.

@@ +725,5 @@
> +        /* Position above click area. */
> +        menuNode.style.top = this.props.eventPosY - menuNodeRect.height - listNodeRect.top - offset + "px";
> +      } else {
> +        /* Position below click area. */
> +        menuNode.style.top = this.props.eventPosY - listNodeRect.top + offset  + "px";

nit: superfluous space.

@@ +734,5 @@
> +      var cx = React.addons.classSet;
> +
> +      return (
> +        <ul className={cx({ "dropdown-menu": true,
> +                            "dropdown-menu-up": this.state.openDirUp })}>

Can you change this to:

```js
var dropdownClasses = React.addons.classSet({
  "dropdown-menu": true,
  "dropdown-menu-up": this.state.openDirUp
});
```

?

::: browser/components/loop/content/shared/js/mixins.js
@@ +100,5 @@
>          };
>        },
>  
> +      /* Event listener callback in charge of closing panels when the users
> +       * clicks on something that is not a dropdown trigger button or menu item. */

nit: Please change this to a proper docblock-style comment.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +197,5 @@
>          }));
>      }
>  
> +    /* Test ConversationDropdown.findNode, read method description if you
> +     * think this does not belong here. */

nit: please change to `// ` comment.

@@ +198,5 @@
>      }
>  
> +    /* Test ConversationDropdown.findNode, read method description if you
> +     * think this does not belong here. */
> +    it("should find `.rooms`", function() {

nit: '' quotes should work as well, no?

@@ +200,5 @@
> +    /* Test ConversationDropdown.findNode, read method description if you
> +     * think this does not belong here. */
> +    it("should find `.rooms`", function() {
> +      /* Ideally we would test finding .rooms from the conversation entry DOM
> +      * node but that dropdown menu is not rendered unless showMenu state. */

nit: please change to `// ` comment.

@@ +613,5 @@
> +        view = mountRoomEntry({room: new loop.store.Room(roomData)});
> +      });
> +
> +      /* XXX Current version of React cannot use TestUtils.Simulate, please enable
> +       * when we upgrade. */

nit: same.

@@ +665,4 @@
>  
> +          sinon.assert.calledOnce(dispatcher.dispatch);
> +          sinon.assert.calledWithExactly(dispatcher.dispatch,
> +                                         new sharedActions.OpenRoom({roomToken: roomData.roomToken}));

nit: please indent with 2 spaces.

@@ +675,4 @@
>  
> +          sinon.assert.calledOnce(dispatcher.dispatch);
> +          sinon.assert.calledWithExactly(dispatcher.dispatch,
> +                                         new sharedActions.OpenRoom({roomToken: roomData.roomToken}));

nit: same.

@@ +808,5 @@
> +        participants: [
> +          { displayName: "Alexis", account: "[e-mail]",
> +            roomConnectionId: "2a1787a6-4a73-43b5-ae3e-906ec1e763cb" },
> +          { displayName: "Adam",
> +            roomConnectionId: "781f012b-f1ea-4ce1-9105-7cfc36fb4ec7" }

nit: please format object literals properly.

@@ +1150,5 @@
> +        participants: [
> +          { displayName: "Alexis", account: "[e-mail]",
> +            roomConnectionId: "2a1787a6-4a73-43b5-ae3e-906ec1e763cb" },
> +          { displayName: "Adam",
> +            roomConnectionId: "781f012b-f1ea-4ce1-9105-7cfc36fb4ec7" }

nit: same.

Comment 12

[Andrei Oprea [:andreio]]

(In reply to Mike de Boer [:mikedeboer] from comment #11)
> Comment on attachment 8653206 [details] [diff] [review]
> Implement context menu for Loop conversation entries
> 
> Review of attachment 8653206 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only r- because of the amount of nits I found! Otherwise, this looks really
> good to me. Please attach a patch with the nits fixed and I'll review it
> asap. Thanks!
> 
> ::: browser/components/loop/content/css/panel.css
> @@ +490,5 @@
> >  .room-list > .room-entry.room-active > h2:before {
> >    background-image: url("../shared/img/icons-14x14.svg#hello-active");
> >  }
> >  
> > +/* Room entry context */
> 
> s/context/dropdown/ ?

Context because it concerns the call button, and chevron not only dropdown.

> 
> @@ +494,5 @@
> > +/* Room entry context */
> > +.room-entry-context-actions {
> > +  display: none;
> > +  border-radius: 30px;
> > +  background: #56b397;
> 
> Did you get this color from a color picker or from the specs?
> 
> @@ +503,5 @@
> > +  display: inline-block;
> > +}
> > +
> > +.room-entry:hover .room-entry-context-actions:hover {
> > +  background: #50e3c2;
> 
> And this one?

Spec

https://www.dropbox.com/home/Fx%20Hello%20Moz-Tef/_Export?preview=CommonEements_Sheet.png

> 
> @@ +509,5 @@
> > +
> > +/* Room entry call button */
> > +.room-entry-call-btn {
> > +  border-top-left-radius: 30px;
> > +  border-bottom-left-radius: 30px;
> 
> For a fully round radius effect, we usually use `radius: 10000px` (since
> 100% doesn't work as you'd expect).
> 

It's in the spec 

https://www.dropbox.com/home/Fx%20Hello%20Moz-Tef/_Export/_Specs?preview=Hello_buttons_spec.png

Comment 13

[Andrei Oprea [:andreio]]

Attachment: Implement context menu for Loop conversation entries

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8653588 [details] [diff] [review]
Implement context menu for Loop conversation entries

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

r=me with comments addressed. Thanks!

::: browser/components/loop/content/js/panel.jsx
@@ +551,5 @@
> +
> +      this.toggleDropdownMenu();
> +    },
> +
> +    /*

nit: Missing '*'

@@ +592,5 @@
>        );
>      }
>    });
>  
>    /*

nit: missing '*'

@@ +670,5 @@
> +      );
> +    }
> +  });
> +
> +  /*

nit: missing '*'.

::: browser/components/loop/content/shared/js/utils.js
@@ +753,5 @@
>  
>      return str;
>    }
>  
> +  /*

nit: missing '*'.

@@ +758,5 @@
> +   * Look up the DOM hierarchy for a node matching `selector`.
> +   * If it is not found return the parent node, this is a sane default so
> +   * that subsequent queries on the result do no fail.
> +   * Better choice than the alternative `document.querySelector(selector)`
> +   * because we ensure it works in the UI showcase as well.

Please document params as well.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +197,5 @@
>          }));
>      }
>  
> +    // Test loop.shared.utils.findParentNode.
> +    // Added here to take advantage of having markup.

Hmmm, not convinced. Can you move these tests to the shared tests as well?

Comment 15

[Andrei Oprea [:andreio]]

Attachment: Implement context menu for Loop conversation entries.

Comment 16

[Andrei Oprea [:andreio]]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/276da0df3947

Comment 18

[Bogdan Maris, Desktop Test Engineering]

Verified that the changes made to Conversation context menu 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. 
No new issues found during testing.