Closed Bug 1071633 Opened 5 years ago Closed 5 years ago

Add dropdown menu to contact buttons

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
3

Tracking

(firefox34 verified, firefox35 verified)

VERIFIED FIXED
mozilla35
Iteration:
35.3
Tracking Status
firefox34 --- verified
firefox35 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: [contacts][first release needed][loop-uplift][fig:wontverify])

Attachments

(1 file, 5 obsolete files)

This bug is about adding the dropdown menu to the button displayed for each contact, according to the UI prototype.
Points: --- → 3
Flags: qe-verify+
Flags: firefox-backlog+
Assignee: nobody → pkerr
WIP: Functioning menu. Needs CSS cleanup.
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.
Attachment #8494903 - Flags: feedback?(paolo.mozmail)
Attachment #8494903 - Flags: feedback?(mdeboer)
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!
Attachment #8494903 - Flags: feedback?(paolo.mozmail)
Attachment #8494903 - Flags: feedback?(mdeboer)
Attachment #8494903 - Flags: feedback+
updated version with css fixed and single dropdown in DOM
Attachment #8494903 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
Attachment #8495686 - Flags: feedback?(paolo.mozmail)
Attachment #8495686 - Flags: feedback?(mdeboer)
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.
Attachment #8495686 - Flags: feedback+
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.
Attachment #8495686 - Flags: feedback?(paolo.mozmail)
Attachment #8495686 - Flags: feedback?(mdeboer)
Attachment #8495686 - Flags: feedback-
> 
> @@ +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.
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.
Attachment #8495686 - Attachment is obsolete: true
Attachment #8496228 - Flags: review?(paolo.mozmail)
Should I open a bug to get a trashcan icon added to the icons-16x16.svg file?
Flags: needinfo?(mdeboer)
Blocks: 1073940
(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.
Depends on: 1038246
Flags: needinfo?(mdeboer)
Attached patch Updated patch (obsolete) — Splinter Review
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!
Assignee: pkerr → paolo.mozmail
Attachment #8496228 - Attachment is obsolete: true
Attachment #8496228 - Flags: review?(paolo.mozmail)
Attachment #8496542 - Flags: review?(mdeboer)
Attached patch Updated with data attributes (obsolete) — Splinter Review
Attachment #8496542 - Attachment is obsolete: true
Attachment #8496542 - Flags: review?(mdeboer)
Attachment #8496834 - Flags: review?(mdeboer)
Blocks: 1038246
No longer depends on: 1038246
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 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.
Attachment #8496834 - Flags: review?(mdeboer)
(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?
(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.
Attachment #8496834 - Attachment is obsolete: true
Attachment #8497384 - Flags: review?(mdeboer)
Comment on attachment 8497384 [details] [diff] [review]
With minimum height

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

LGTM, thanks!
Attachment #8497384 - Flags: review?(mdeboer) → review+
Iteration: --- → 35.3
https://hg.mozilla.org/mozilla-central/rev/95aefa6ce37f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Dropping from Fig verifications but will verify in Nightly.
Whiteboard: [contacts][first release needed][loop-uplift] → [contacts][first release needed][loop-uplift][fig:wontverify]
Pauly -- This is ready to be verified. Thanks!
Flags: needinfo?(paul.silaghi)
Verified fixed FF 35.0a1 (2014-10-06) Win 7, Ubuntu 13.04, OS X 10.10
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Comment on attachment 8497384 [details] [diff] [review]
With minimum height

Approval Request Comment
Part of the staged Loop aurora second uplift set
Attachment #8497384 - Flags: approval-mozilla-aurora?
Flags: needinfo?(paul.silaghi)
Verified fixed FF 34b1 Win 7
Flags: needinfo?(paul.silaghi)
Comment on attachment 8497384 [details] [diff] [review]
With minimum height

Already landed in aurora, approving for posterity
Attachment #8497384 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.