Make Loop Incoming Call panel match the MVP spec

VERIFIED FIXED in 33 Sprint 3- 7/21

Status

defect
P1
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: aoprea, Assigned: aoprea)

Tracking

unspecified
33 Sprint 3- 7/21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=1][qa+])

Attachments

(2 attachments, 1 obsolete attachment)

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
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → 33 Sprint 3- 7/21
Carrying over estimation from the bug this was spun out of.
Priority: -- → P1
Whiteboard: [p=1]
Comment on attachment 8458946 [details] [diff] [review]
Make Loop Incoming call match the MVP spec

Patch from Andrei, setting review to me...
Attachment #8458946 - Flags: review?(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.
Attachment #8458946 - Flags: review?(dmose) → review+
Attachment #8458946 - Attachment is obsolete: true
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
Attachment #8459002 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/db93dd7269d2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
I've verified this bug is fixed in today's Nightly and Aurora builds.
Status: RESOLVED → VERIFIED
QA Contact: anthony.s.hughes
Whiteboard: [p=1] → [p=1][qa+]
You need to log in before you can comment on or make changes to this bug.