1043347 - [Dialer][Call Screen] Bad alignment of information in conference call overlay

Status: RESOLVED FIXED

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

Description

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: conferencecall_overlay_number.png

STR:
1. Establish a conference call (group call with at least 2 other participants).
2. Click on the call screen header ("Conference (N)") to get the participant info overlay.

EXPECTED:
3. The participant info and hang up icon are properly aligned and shown for the number and contact cases.

OBSERVED:
3. The participant info and hang up icon are NOT properly aligned either for the number (I guess the number should be vertically centered and aligned with the hang up icon) or and contact cases (I guess the name and phone info should be vertically centered on the available space and also with the hang up icon).

Comment 1

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: conferencecall_overlay_contact.png

Screenshot for the Contact information case ;)

Comment 2

[Germán Toro del Valle (:gtorodelvalle)]

Hi Carol, I found https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8421731 ("Visual Mockup. Conference Call. List of participants one Call ended") in bug 950760 but it would be great to have concrete specs (such as https://bug950760.bugzilla.mozilla.org/attachment.cgi?id=8415257) to implement the mentioned scenarios ;) Would it be possible? Thanks!

Comment 3

[Germán Toro del Valle (:gtorodelvalle)]

Although probably not strictly blocking it, it is highly related and both bugs should be considered in parallel to implement the desired look and feel :)

Comment 4

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: 22172.html

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: [screenshot][proposal] conferencecall_overlay_number_and_contact.png

Hi Carol, I have a patch with what I considered is the desired behaviour so that the specs would not be needed. Would you be so kind to ui-review it, please? Thanks!

Comment 6

[Carol Huang [:Carol]]

Comment on attachment 8462581 [details]
[screenshot][proposal] conferencecall_overlay_number_and_contact.png

Hi,
The adjustment looks pretty good, I'll give UI+
Thank you!! :)

Comment 7

[Anthony Ricaud (:rik)]

I think this patch will change a lot after we land bug 967440. So I'll wait for that to review.

Comment 8

[Anthony Ricaud (:rik)]

Comment on attachment 8462580 [details]
22172.html

This is violating the separation of concerns between the call screen and the handled call. Handled calls should not know about the DOM of the call screen (and vice versa).

What you want here is to check this.call.group in HandledCall.formatPhoneNumber. When it exists, you do this.node.style.fontSize = ''.

Doug: Needinfo-ing you to spread knowledge about this separation of concerns for later reviews.

Comment 9

[Doug Sherk (:drs) (inactive)]

Got it, thanks.

Comment 10

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to Anthony Ricaud (:rik) from comment #8)
> Comment on attachment 8462580 [details]
> 22172.html
> 
> This is violating the separation of concerns between the call screen and the
> handled call. Handled calls should not know about the DOM of the call screen
> (and vice versa).
> 
> What you want here is to check this.call.group in
> HandledCall.formatPhoneNumber. When it exists, you do
> this.node.style.fontSize = ''.
> 
> Doug: Needinfo-ing you to spread knowledge about this separation of concerns
> for later reviews.

Well, taking into consideration the current implementation I would say each instance of HandledCall knows (and holds a lot of info) about the DOM: https://github.com/mozilla-b2g/gaia/blob/master/apps/callscreen/js/handled_call.js#L32-L58 :p Kidding! I agree with your proposal ;)

I'll update que PR in a sec ;)

Comment 11

[Anthony Ricaud (:rik)]

Comment on attachment 8462580 [details]
22172.html

Left my comments on Github.

Comment 12

[Germán Toro del Valle (:gtorodelvalle)]

Hi Anthony, I left a comment to your comment in Github :)

Comment 13

[Anthony Ricaud (:rik)]

Agreed on the approach suggested in Github.

Comment 14

[Germán Toro del Valle (:gtorodelvalle)]

Comment on attachment 8462580 [details]
22172.html

Patch including the agreed combined approach as per comment 13 ;)

Comment 15

[Germán Toro del Valle (:gtorodelvalle)]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/286383c71e6e85863a8f304e0a7f9ac1032df109

Comment 16

[Loli (:lolimartinezcr)]

An bug prevents to test it:

https://bugzilla.mozilla.org/show_bug.cgi?id=1053138