Closed Bug 1073469 Opened 8 years ago Closed 7 years ago

Include contact views in the UI showcase

Categories

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

defect

Tracking

(Not tracked)

RESOLVED WORKSFORME
backlog tech-debt

People

(Reporter: Paolo, Unassigned)

References

Details

(Whiteboard: [tech-debt])

Attachments

(1 file, 3 obsolete files)

The Loop Desktop client needs to include the contact views in the UI showcase.
Attached patch Paul's work in progress (obsolete) — Splinter Review
Assignee: nobody → pkerr
Status: NEW → ASSIGNED
Attachment #8496971 - Attachment is obsolete: true
Attachment #8496541 - Attachment is obsolete: true
WIP: update to menu operation in showcase
Attachment #8496976 - Attachment is obsolete: true
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?
Attachment #8497844 - Flags: feedback?(paolo.mozmail)
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.
Attachment #8497844 - Flags: feedback?(paolo.mozmail)
> Do the getElementsByClassName work in your build?

It works, but it assumes there is only one item with that class.
(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.
(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.
backlog: --- → Fx36?
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.
backlog: Fx36? → Fx37?
Flags: needinfo?(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.
Flags: needinfo?(pkerr)
backlog: Fx37? → Fx38?
Assignee: pkerr → nobody
Status: ASSIGNED → NEW
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?
Flags: needinfo?(nperriault)
Priority: -- → P5
Whiteboard: [tech-debt]
(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.
Flags: needinfo?(nperriault)
(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.
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.
(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 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.
Attachment #8497844 - Flags: feedback-
(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.
(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!
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).
backlog: Fx38? → tech-debt
We now have contact views in the ui-showcase -> WFM.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.