1059336 - Add bindings for contact buttons in the call information page

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[Doug Sherk (:drs) (inactive)]

Attachment: Sample deployment of contacts buttons in dialer.

In bug 877971, we're adding a call log information page. We need to add bindings here for the buttons that we're refactoring out of the contacts app in bug 1054995. I've attached an example of how to do this without the required work in bug 1047353.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: WIP: Add bindings for contact buttons in the call information page.

PR: https://github.com/mozilla-b2g/gaia/pull/23395

This has some problems and I haven't written any tests yet. To really iron this out, we'll have to get bug 1047353 landed first.

Comment 2

[Doug Sherk (:drs) (inactive)]

Attachment: Part 2: Minor tweaks to shared contacts buttons code.

PR: https://github.com/mozilla-b2g/gaia/pull/23395

Francisco, you can check the PR, but it's pretty messy since it's stacked on top of 2 other WIP bugs. I'm not happy about the changes needed here, but there are slight differences in the DOMs of contacts and dialer which are affecting how these elements are displayed. I expect that some of these problems will go away once we have bug 1060076 fixed.

Comment 3

[Doug Sherk (:drs) (inactive)]

Attachment: Part 1: Add bindings for contact buttons in the call information page.

Updated PR.

The PR is a mess because of all of the WIP patches it's based on top of. I'll make sure to clean it up before we go into the final round of review. It will require a rebase on top of bug 1047353.

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8481204 [details] [diff] [review]
Part 2: Minor tweaks to shared contacts buttons code.

You'll need to rebase the patch, just the xfail.list, so I applied the patch without this file and working good in contacts.

r+ for this one.

Thanks Doug!

Comment 5

[Anthony Ricaud (:rik)]

Comment on attachment 8481355 [details] [diff] [review]
Part 1: Add bindings for contact buttons in the call information page.

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

Very good! I think we're just missing one test and some nitpicking on the HTML.

::: apps/communications/dialer/elements/call-info-view.html
@@ +17,5 @@
>        </div>
>      </div>
>  
> +    <div class="ci__details-wrapper">
> +      <section id="contact-detail">

I'd rather move this inside .ci__contact-wrapper. And use a <div>

Like:
<div class="ci__contact-wrapper">
  <div id="contact-detail">…</section>
  <div id="contact-actions">…</div>
</div>

::: apps/communications/dialer/js/call_info.js
@@ +216,5 @@
> +                       '/dialer/js/telephony_helper.js',
> +                       '/shared/style/contacts/contacts_buttons.css',
> +                       '/shared/style/contacts.css',
> +                       '/dialer/style/buttons.css'], function() {
> +        if (!self._initialised) {

With this, we can remove var _initialised = false; that was used for initListeners.

::: apps/communications/dialer/test/unit/call_info_test.js
@@ +76,5 @@
> +
> +    initCallInfo();
> +  });
> +
> +  suiteSetup(function() {

Please move the suiteSetup and suiteTeardown before setup().

@@ +139,5 @@
> +      assert.isNull(document.getElementById('email-details-stub'));
> +    });
> +  });
> +
> +  suite('ContactsButton', function() {

I think if you remove listDetailsElt.innerHTML = ''; from call_info.js, those tests will still pass. We need one more test to check this. You could stub renderPhones and renderButtons to return something and verify that the number of elements matches what's expected.

Comment 6

[Doug Sherk (:drs) (inactive)]

Attachment: Part 1: Add bindings for contacts buttons in the call information page.

Updated PR. I couldn't rebase since the underlying WIPs haven't seen enough progress yet.

(In reply to Anthony Ricaud (:rik) from comment #5)
> With this, we can remove var _initialised = false; that was used for
> initListeners.

This was already done, unless I missed something.

> I think if you remove listDetailsElt.innerHTML = ''; from call_info.js,
> those tests will still pass.

If I remove this line, each time the view is shown, it will include the buttons from the last displaying.

> We need one more test to check this. You could
> stub renderPhones and renderButtons to return something and verify that the
> number of elements matches what's expected.

This seems pointless to me. ContactsButtons already includes tests for this. All we'll be verifying is that given a mock contact, we pass in that mock contact in its entirety, which there's already a test for.

Comment 7

[Doug Sherk (:drs) (inactive)]

Attachment: Part 1: Add bindings for contacts buttons in the call information page.

Updated PR.

I misunderstood your point about listDetailsElt.innerHTML = ''. I added a test to make sure that we clear it on every ContactsButtons.show() call.

Comment 8

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/42424070ffaf6a5b792b63ea0f5a780d46546c1e