1073469 - Include contact views in the UI showcase

Status: RESOLVED WORKSFORME

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

Description

[:Paolo Amadini]

The Loop Desktop client needs to include the contact views in the UI showcase.

Comment 1

[:Paolo Amadini]

Attachment: Paul's work in progress

Comment 2

[Paul Kerr [:pkerr]]

Attachment: Include contact views in the UI showcase

WIP

Comment 3

[Paul Kerr [:pkerr]]

Attachment: Include contact views in the UI showcase

WIP

Comment 4

[Paul Kerr [:pkerr]]

Attachment: Include contact views in the UI showcase

WIP: update to menu operation in showcase

Comment 5

[Paul Kerr [:pkerr]]

Comment on attachment 8497844 [details] [diff] [review]
Include contact views in the UI showcase

Paolo, please look at the code change I made in the ContractDropdown.componentDidMount() method. When running in the showcase on the ui server, the call in your patch:
let listNode = document.getElementsByClassName("contact_list")[0];
was returning a completely disconnected element. That is, it had no child elements and the getBoundingClientRect() returned as all 0. I did some checking in the debugger and if I followed the offsetParent.offsetParent chain I got to a .content-list class element but it showed 6 child elements. Checking the data-reactid of both list elements showed they were different and the one returned from the offsetParent chain matched that of the generated html shown in the debugger window. Based on this, I think that the getElementsByClassName() is not returning the React backing instance but something from the React DOM copy.

The intent of the code is to flip the dropdown menu to extend upwards when it would drop below the list box. I cannot get the flip to work in the showcase without reverting the code back to using the offsetParent chain. Do the getElementsByClassName work in your build?

Comment 6

[:Paolo Amadini]

Comment on attachment 8497844 [details] [diff] [review]
Include contact views in the UI showcase

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

I can't look into this in detail right now, but I have a quick comment that may help in moving this forward.

::: browser/components/loop/content/js/contacts.js
@@ +36,5 @@
>        let menuNode = this.getDOMNode();
>        let menuNodeRect = menuNode.getBoundingClientRect();
>  
> +      let lineNode = menuNode.offsetParent;
> +      let listNode = lineNode.offsetParent;

This, unfortunately, is a bit fragile as it depends on the CSS classes. You should probably ask for feedback from NiKo or Dan about how to get this reference in React.

It would be useful to get one render of the view with the context menu open on a top contact and on a bottom contact, if possible.

Comment 7

[:Paolo Amadini]

> Do the getElementsByClassName work in your build?

It works, but it assumes there is only one item with that class.

Comment 8

[Paul Kerr [:pkerr]]

(In reply to :Paolo Amadini from comment #7)
> > Do the getElementsByClassName work in your build?
> 
> It works, but it assumes there is only one item with that class.

Yes, there appears to be only one .contact-list in the DOM. When I temporarily tried using an id attribute (#contacts) I got the same result as with the class name look-up.

Comment 9

[Paul Kerr [:pkerr]]

(In reply to :Paolo Amadini from comment #6)
> Comment on attachment 8497844 [details] [diff] [review]
> Include contact views in the UI showcase
> 
> Review of attachment 8497844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can't look into this in detail right now, but I have a quick comment that
> may help in moving this forward.
> 
> ::: browser/components/loop/content/js/contacts.js
> @@ +36,5 @@
> >        let menuNode = this.getDOMNode();
> >        let menuNodeRect = menuNode.getBoundingClientRect();
> >  
> > +      let lineNode = menuNode.offsetParent;
> > +      let listNode = lineNode.offsetParent;
> 
> This, unfortunately, is a bit fragile as it depends on the CSS classes. You
> should probably ask for feedback from NiKo or Dan about how to get this
> reference in React.
> 
> It would be useful to get one render of the view with the context menu open
> on a top contact and on a bottom contact, if possible.

This is only a temporary debug version.

Comment 10

[:shell escalante]

moving to 37 - removing assignee since assuming this is available for picking.  wanted to verify there wasn't other work pending not on bug or review needed.

Comment 11

[Paul Kerr [:pkerr]]

No other bugs are blocked by this. I was investigating why code that worked in the Loop product did not work within the showcase.

Comment 12

[:shell escalante]

Hi Niko, Since this bug is older and there was some work - figured I'd ask you if you knew if Contacts View made it into UI Showcase or should be left open as tech-debt to do.  Also relative to other bugs - how painful is not having contacts in UI showcase?

Comment 13

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

(In reply to sescalante from comment #12)
> Hi Niko, Since this bug is older and there was some work - figured I'd ask
> you if you knew if Contacts View made it into UI Showcase or should be left
> open as tech-debt to do.  Also relative to other bugs - how painful is not
> having contacts in UI showcase?

Works has already started with bug 1000131, which landed ContactList view instances in the showcase to show the notifications, but the list itself is empty. It should be trivial to add some fake contact entries in there though. I'd suggest to leave this bug open.

Comment 14

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

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #13)
> (In reply to sescalante from comment #12)
> Works has already started with bug 1000131

Sorry, I meant bug 1076764.

Comment 15

[Paul Kerr [:pkerr]]

There is code in the attached patch that will populate the contact list. The blocking issue was that the code in the ContactDropDown::componentDidMount that used to walk the chain of offsetParent attributes to find the enclosing list was replaced with a call to getElementsByClassName("contact-list")[0] in another patch (see Paolo's comment above as to why the change was made). The getElementsByClassName call works fine in the Hello client in the browser but returns null when run in the showcase. The original code sequence worked for both. Why the second form does not work in the showcase environment I never got a chance to work out before I moved on to fix higher priority bugs.

Comment 16

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

(In reply to Paul Kerr [:pkerr] from comment #15)
> There is code in the attached patch that will populate the contact list. 

Sorry Paul, I totally missed a patch was attached :x

I can have a look at it next week.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8497844 [details] [diff] [review]
Include contact views in the UI showcase

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

I'm sorry, Paul, but I'll be fixing this in bug 1069962 in a different way.

Comment 18

[Paul Kerr [:pkerr]]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Comment on attachment 8497844 [details] [diff] [review]
> Include contact views in the UI showcase
> 
> Review of attachment 8497844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm sorry, Paul, but I'll be fixing this in bug 1069962 in a different way.

Mike, the attached but is not a proposed fix by me. It is was a work-in-progress patch that I put up for feedback from Paolo (when the bug was first opened). Paolo then took it an landed a revised version of the patch. I simply wanted to point out where one thing worked in the showcase and another API set did not.

Comment 19

[Mike de Boer [:mikedeboer]]

(In reply to Paul Kerr [:pkerr] from comment #18)
> Mike, the attached but is not a proposed fix by me. It is was a
> work-in-progress patch that I put up for feedback from Paolo (when the bug
> was first opened). Paolo then took it an landed a revised version of the
> patch. I simply wanted to point out where one thing worked in the showcase
> and another API set did not.

Ohhh, pardon, don't mind me then!

Comment 20

[:shell escalante]

Moving these to blocking-loop="tech-debt" so they can be triaged against other tech debt bugs.  When we take a tech-debt bug to work on - just mark it "firefox-backlog"+ and give priority.  Then we'll pull it during iteration planning (or factor it into workload if someone is already working on).

Comment 21

[Mark Banner (:standard8)]

We now have contact views in the ui-showcase -> WFM.