1071633 - Add dropdown menu to contact buttons

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[:Paolo Amadini]

This bug is about adding the dropdown menu to the button displayed for each contact, according to the UI prototype.

Comment 1

[Paul Kerr [:pkerr]]

Work in progress at: https://github.com/dmose/gecko-dev/tree/contacts-dropdown-1071633

Comment 2

[Paul Kerr [:pkerr]]

Attachment: WIP Add dropdown menu to button on contact details line

WIP: Functioning menu. Needs CSS cleanup.

Comment 3

[Maire Reavy [:mreavy]]

Comment on attachment 8494903 [details] [diff] [review]
WIP Add dropdown menu to button on contact details line

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

Mike, Paolo -- Paul and Dan made good progress today working together on this patch.  I know Paul wanted to ask for feedback from you so he'd have it for the start of his day tomorrow, so I'm putting this up on his behalf.  Thanks.

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8494903 [details] [diff] [review]
WIP Add dropdown menu to button on contact details line

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

<3 that the showcase got updated in this patch!

The problem with this patch is that right now it creates dropdowns for each and every contact. If the list contains, say, 500 contacts, then that's an aweful lot of DOM nodes.

In other words; what I'd like to see is one dropdown in the ContactsList that shows up at the right position when the call button is clicked.
That leads me to my second point: there's also the issue that the dropdown needs to be positioned correctly when it partly disappears due to overflow of the list when many contacts are listed.

Feel free to ping me on IRC to discuss this further!

Comment 5

[Paul Kerr [:pkerr]]

Attachment: Add dropdown menu to button on contact details line

updated version with css fixed and single dropdown in DOM

Comment 6

[Paul Kerr [:pkerr]]

Comment on attachment 8495686 [details] [diff] [review]
Add dropdown menu to button on contact details line

Menu actions can be rooted on the ContextDropdown component in contact.jsx. Please take a look.

Comment 7

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8495686 [details] [diff] [review]
Add dropdown menu to button on contact details line

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

This is super good. A few nits though.

::: browser/components/loop/content/js/contacts.jsx
@@ +72,5 @@
>      },
>  
> +    renderDropdown: function() {
> +      this.showDropdownMenu();
> +      return;

Unnecessary return statement. Also, this method is probably unneeded (keep reading).

@@ +102,5 @@
>                <i className={cx({"icon icon-blocked": this.props.contact.blocked})} />
>              </div>
>              <div className="email">{email.value}</div>
>            </div>
> +          <div className="icons" onClick={this.renderDropdown}>

You can directly use this.showDropdownMenu here.

@@ +107,4 @@
>              <i className="icon icon-video" />
>              <i className="icon icon-caret-down" />
>            </div>
> +          {dropdown}

{this.state.showMenu ? <Dropdown/> : null}

@@ +123,1 @@
>        };

How about

return {
  contacts: this.props.contacts || {}
};

::: browser/components/loop/content/shared/css/contacts.css
@@ +97,5 @@
> +  top: 10px;
> +  right: 1em;
> +  left: auto;
> +  z-index: 1000;
> +} 

Nit: trailing space.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +128,5 @@
> +      "_date_lch": 1411582790958,
> +      "_date_add": 1411582790958,
> +      "_guid": 5
> +    }
> +  };

These fixtures want to move to some dedicated file we could import.

Comment 8

[:Paolo Amadini]

Comment on attachment 8495686 [details] [diff] [review]
Add dropdown menu to button on contact details line

I think the code looks good and NiKo already provided feedback on it, but from the CSS / UX standpoint this needs work, as the menu can still be partially hidden when opened on a contact in the lower part of the list.

Comment 9

[Paul Kerr [:pkerr]]

> 
> @@ +102,5 @@
> >                <i className={cx({"icon icon-blocked": this.props.contact.blocked})} />
> >              </div>
> >              <div className="email">{email.value}</div>
> >            </div>
> > +          <div className="icons" onClick={this.renderDropdown}>
> 
> You can directly use this.showDropdownMenu here.
> 
> @@ +107,4 @@
> >              <i className="icon icon-video" />
> >              <i className="icon icon-caret-down" />
> >            </div>
> > +          {dropdown}
> 
> {this.state.showMenu ? <Dropdown/> : null}
> 

Sure. The indirection is a left-over from an intermediate version I was working on. I should have removed it earlier.

Comment 10

[Paul Kerr [:pkerr]]

Attachment: Add dropdown menu to button on contact details line

Added code to open the menu upwards when it would have extended below the bottom of the list. Placed dummy users for showcase into separate file.

Comment 11

[Paul Kerr [:pkerr]]

Should I open a bug to get a trashcan icon added to the icons-16x16.svg file?

Comment 12

[:Paolo Amadini]

(In reply to Paul Kerr [:pkerr] from comment #11)
> Should I open a bug to get a trashcan icon added to the icons-16x16.svg file?

Filed bug 1073940.

Comment 13

[:Paolo Amadini]

Attachment: Updated patch

Hi Paul, thanks for the patch. I updated it in various ways to drive it to a state that is nearly complete, and I'll ask Mike for the next review pass.

This patch applies on top of bug 1038246.

I had to detach the code from the shared dropdown mixin because I found subtle bugs in it as soon as I added the click handlers. The current situation is less than ideal, but I think it solves the problems I found.

I moved the UI showcase part to a separate bug that was already on file, bug 1073469, in order to reduce the amount of work required for landing this UI. If you would like to continue working on that bug, it will be appreciated!

Comment 14

[:Paolo Amadini]

Attachment: Updated with data attributes

Comment 15

[:Paolo Amadini]

Comment on attachment 8496834 [details] [diff] [review]
Updated with data attributes

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

::: browser/components/loop/content/shared/css/contacts.css
@@ -43,5 @@
>  }
>  
>  .contact:hover > .icons {
>    display: block;
> -  z-index: 1000;

This caused a regression with the overlap with the "imported" indicator, so I just changed this to "1" and the z-index of the menu to "2".

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8496834 [details] [diff] [review]
Updated with data attributes

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

Paolo, please see my comment below... what do you think? I cancelled review for the moment.

::: browser/components/loop/content/js/contacts.jsx
@@ +35,5 @@
> +
> +      let menuNode = this.getDOMNode();
> +      let menuNodeRect = menuNode.getBoundingClientRect();
> +
> +      let listNode = document.getElementsByClassName("contact-list")[0];

I just tested this patch and noticed a different bug: when I have only one contact in the list, the dropdown disappears below the 'Import' and 'New Contact' buttons.
I think we need to up the z-index of this menu and have a different overflow parent, or think of something else.
Or we determine this is a UX issue and we need to implement something different than a dropdown in a follow-up.

Comment 17

[Paul Kerr [:pkerr]]

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> Comment on attachment 8496834 [details] [diff] [review]
> Updated with data attributes
> 
> Review of attachment 8496834 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Paolo, please see my comment below... what do you think? I cancelled review
> for the moment.
> 
> ::: browser/components/loop/content/js/contacts.jsx
> @@ +35,5 @@
> > +
> > +      let menuNode = this.getDOMNode();
> > +      let menuNodeRect = menuNode.getBoundingClientRect();
> > +
> > +      let listNode = document.getElementsByClassName("contact-list")[0];
> 
> I just tested this patch and noticed a different bug: when I have only one
> contact in the list, the dropdown disappears below the 'Import' and 'New
> Contact' buttons.
> I think we need to up the z-index of this menu and have a different overflow
> parent, or think of something else.
> Or we determine this is a UX issue and we need to implement something
> different than a dropdown in a follow-up.

I had tried upping the z-index at one point but the scroll setting of the list kept capturing the dropdown menu. Should the scroll be held off until the list has more than a few items in the list?

Comment 18

[:Paolo Amadini]

Attachment: With minimum height

(In reply to Mike de Boer [:mikedeboer] from comment #16)
> I just tested this patch and noticed a different bug: when I have only one
> contact in the list, the dropdown disappears below the 'Import' and 'New
> Contact' buttons.

Thanks for catching this!

(In reply to Paul Kerr [:pkerr] from comment #17)
> I had tried upping the z-index at one point but the scroll setting of the
> list kept capturing the dropdown menu. Should the scroll be held off until
> the list has more than a few items in the list?

I think the easiest solution is to enforce a minimum height for the list, so that we don't need to render the menu elements outside of the list itself.

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8497384 [details] [diff] [review]
With minimum height

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

LGTM, thanks!

Comment 20

[:Paolo Amadini]

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

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/95aefa6ce37f

Comment 22

[u279076]

Dropping from Fig verifications but will verify in Nightly.

Comment 23

[Maire Reavy [:mreavy]]

Pauly -- This is ready to be verified. Thanks!

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35.0a1 (2014-10-06) Win 7, Ubuntu 13.04, OS X 10.10

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8497384 [details] [diff] [review]
With minimum height

Approval Request Comment
Part of the staged Loop aurora second uplift set

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4bc85cfd6e7

Comment 27

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b1 Win 7

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8497384 [details] [diff] [review]
With minimum height

Already landed in aurora, approving for posterity