1040901 - Make Loop Incoming Call panel match the MVP spec

Status: VERIFIED FIXED

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

Description

[Andrei Oprea [mention me on [e-mail]]]

Introduce (part) of the new UI [0] that is in preparation for having audio only or audio+video calls bug 990678

[0] https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#start-call-account

Comment 1

[Andrei Oprea [mention me on [e-mail]]]

Attachment: Screen Shot 2014-07-18 at 1.30.03 PM.png

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Carrying over estimation from the bug this was spun out of.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Make Loop Incoming call match the MVP spec

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8458946 [details] [diff] [review]
Make Loop Incoming call match the MVP spec

Patch from Andrei, setting review to me...

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8458946 [details] [diff] [review]
Make Loop Incoming call match the MVP spec

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

Looks good, r=dmose with the commented changes.

::: browser/components/loop/content/js/conversation.js
@@ +35,5 @@
> +      var platform="unknown_platform";
> +
> +      if (navigator.platform.indexOf("Win") != -1) platform = "windows";
> +      if (navigator.platform.indexOf("Mac") != -1) platform = "mac";
> +      if (navigator.platform.indexOf("Linux") != -1) platform = "linux";

Please put the "then" statements on braced separate lines (prevailing style).

::: browser/components/loop/content/shared/css/common.css
@@ +145,5 @@
> +  color: #fff;
> +  font-size: 1.1em;
> +  line-height: 26px;
> +  border-left: 1px solid rgba(255, 255, 255, .3);
> +}

Since this isn't currently being used, let's remove it.

@@ +219,5 @@
> +
> +.light {
> +  color: rgba(51, 51, 51, .5);
> +}
> +

How about a comment here explaining that the reasoning for both the OS-specific classes and the very specific font rules (eg in px) is because this way we can have confidence of blending in well with OS look-and-feel.

::: browser/components/loop/content/shared/css/conversation.css
@@ +158,5 @@
> +  display: flex;
> +  flex-direction: column;
> +  align-items: center;
> +  justify-content: space-between;
> +  min-height: 264px;

Needs an XXX comment that this is the exact height of the docked window, but looks bad in the popped case and needs fixing.  Would be good to file a bug for this as well.

@@ +174,3 @@
>  }
>  
>  .incoming-call button {

Maybe .incoming-call .btn for consistency with our other CSS.

@@ +174,4 @@
>  }
>  
>  .incoming-call button {
> +  flex: 1;

Let's either make this go away, or at least comment on its purpose.

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +41,3 @@
>      };
> +
> +    document.mozL10n.initialize(navigator.mozLoop);

Please comment on why this is necessary, and file a bug to hoist this stub into a shared utility function file somewhere.

@@ +329,3 @@
>      });
>  
>      describe("#handleAccept", function() {

This is no longer testing "#handleAccept", it's testing "click event on .btn-accept.

@@ +330,5 @@
>  
>      describe("#handleAccept", function() {
> +      it("should trigger an 'accept' conversation model event", function() {
> +        var buttonAccept = view.getDOMNode()
> +                                    .querySelector(".btn-accept");

This will all fit on one line; please check this and the other querySelector changes for correct line-breaking and indentation.

@@ +340,4 @@
>          });
>      });
>  
>      describe("#handleDecline", function() {

Please rename this block similarly to the last one.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Make Loop incoming call view match MVP spec

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8459002 [details] [diff] [review]
Make Loop incoming call view match MVP spec

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

Carrying forward r=dmose

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db93dd7269d2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db93dd7269d2

Comment 10

[u279076]

I've verified this bug is fixed in today's Nightly and Aurora builds.