1048938 - Update Loop conversation layout to match latest design from the mockups

Status: VERIFIED FIXED

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

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Spent some time earlier going over what we're hoping for with Darrin, and sent out a mail to help the process of seeing if we can get some time from him.

The Etherpad from this morning is at <https://bugzilla.mozilla.org/show_bug.cgi?id=1048938>.

Darrin suggested simply leaving the detached keeping the designs shared between the attached and detached windows, and effectively using similar/identical styles from them.  I.e. making the detached versiojn minimally responsive, rather than trying to include of the link-clicker responsivity.

Everyone agreed, I think, that spending a half day or day trying to do a simple proof-of-concept before moving forward on the production code would be a good way to go.

Mike, I could imagina you starting that on Monday before the rest of us are online, or simply waiting until we hear back more about Darrin's time later in the day.  Your call...

My Monday morning is flexible, I'd be happy to start pairing on it over hangouts as soon as we finish standup, if you think that'd be helpful.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

I haven't had time to move this forward today (sorry!), but tomorrow I should have more time.  In the meantime, the work we did this morning is here:

https://github.com/dmose/gecko-dev/tree/visual-port-spike

And Darrin pushed his repo for the link-clicker source here:

https://github.com/darrinhenein/mozilla-loop-web/tree/master/src

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

My thought for the next step in the spike would be to let go of the desktop UX for now, and move forward on the standalone stuff, now that we've got the standalone source.  This seems like it will provide the most information quickest about exactly what is going to be port-worthy.

So far, I'm finding the existing CSS in the main tree to be complex enough to unravel that I'm wondering if it would be faster to throw out all of CSS in the main tree, apply reset.css, and then attempt to do the porting trick into a completely (mostly?) empty tree, rather than into the tangle of existing CSS rules.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(I have no great degree of confidence that the above would be faster, because there's enough flex-box magic semantics expected by (particularly the dropdown) markup that we've got that I suspect rebuilding that would be slow.  I mostly injected the above because it feels worthy of a bit of though, or maybe even a mini-spike.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Shortly after Mike and I spent some time pow-wowing with Darrin, I remembered Niko's original branch over at:

https://github.com/n1k0/gecko-dev/commit/b070ffad05941dcb5f1fe2167916a1380c6fa340#diff-e7c98d3746ec665b30a13e1d26322c44R156

He was doing pretty much exactly the same stuff that Mike and I had agreed were next steps, and had made some significant progress.  So I decided to rebase our reset.css changes to the latest fx-team, and then port Niko's
to live on top of that.  The results of that have been (force-)pushed to:

https://github.com/dmose/gecko-dev/compare/dmose:fx-team...visual-port-spike

Things look pretty good in the ux-showcase, but less good in the real standalone and desktop-local embeddings.  If you look at the changes, you'll see a bunch of YYY comments that need to be investigated; these are things that I carried over from the port, but have some issues.  The next step, I suspect, is to debug why the showcase doesn't emulate the embeddings well enough, and fix that.

After doing this work, I'm starting to have a lot more sympathy for Niko's desire to insert something to make it easier to keep our CSS modular.  As it standards, conversation.css feels really pretty fragile.  Hmmm...

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Since there are lots of little niggling bits of this work, Mike and I have agreed to use https://webrtc.etherpad.mozilla.org/27 to track them.

Comment 7

[Maire Reavy [:mreavy]]

With Mike on PTO, Dan has taken over driving this.

Comment 8

[Maire Reavy [:mreavy]]

Marking the iteration as 34.3 since that's the bugzilla field we're using now

Comment 9

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

Attachment: Part 1 of visual uplift

Comment 10

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

Comment on attachment 8477023 [details] [diff] [review]
Part 1 of visual uplift

Please read Draft Style Guide section [0] of the document, it captures some of the code style decisions we took. These are decisions that seemed reasonable at the time, but can change, I would like to hear your thoughts on it. 

The high level feedback should only regard the CSS files. All other changes were made to accommodate the CSS changes (either markup updates or test updates)

[0] https://webrtc.etherpad.mozilla.org/27

Comment 11

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

For example, common.css captures the styleguide decisions best. The rest of the CSS files might have rules that break the guide because it would require refactoring the components as well, and it felt like this patch became too bloated already to go down that path.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

@n1k0: note that the intent is not at all to do a full-blown review of that style here, as we've been more or less reviewing as we go.  Rather, we're looking for basic confirmation that this is a reasonable next step in broad strokes.  @tOkeshu should be able to give you more context...

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8477023 [details] [diff] [review]
Part 1 of visual uplift

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

I was surprised to find most of the code I initially write as a spike here https://github.com/n1k0/gecko-dev/tree/bug-1048938-update-conversation-layout, while I thought we'd want to investigate using a different box model, especially for when it comes to responsive stuff — as showcased in :darrin's mockups.

Please not I could not apply the patch locally for testing (it needs rebasing against latest master/fx-team), so this review is mostly theoretical — hence feedback- until I can actually apply the patch cleanly :)

::: browser/components/loop/content/js/conversation.jsx
@@ +81,2 @@
>        var btnClassDecline = "btn btn-error btn-decline";
> +      var conversationPanelClass = "incoming-call";

Assigning these class names to vars makes no sense to me; as they never change, they all should be applied inline in jsx.

@@ +89,5 @@
>          <div className={conversationPanelClass}>
>            <h2>{__("incoming_call")}</h2>
> +          <div className="btn-group incoming-call-action-group">
> +
> +            <div className="fx-embedded-incoming-call-button-spacer"></div>

This reminds me the good old spacer.gif. Can't we just use margin to add space here?

@@ +113,5 @@
>  
>                </div>
>              </div>
>  
> +            <div className="fx-embedded-incoming-call-button-spacer"></div>

Same remark as above.

@@ +132,5 @@
>                  </div>
>                </div>
>              </div>
> +
> +            <div className="fx-embedded-incoming-call-button-spacer"></div>

Same remark as above.

::: browser/components/loop/content/shared/css/common.css
@@ +52,5 @@
>   */
>  .cf:before,
>  .cf:after {
> +  display: table; /* 1 */
> +  content: " ";   /* 2 */

This should be reverted as now the pointers don't match anymore.

@@ +218,2 @@
>    display: flex;
> +  /*width: 100%;*/

nit: if no longer needed, this should be removed.

::: browser/components/loop/content/shared/css/conversation.css
@@ +54,5 @@
>  
> +  .standalone .conversation-toolbar li {
> +    /* XXX Might make sense to use relative units here.
> +     */
> +    margin-right: 16px;

You'll probably get told that this is not RTL-ready, so it might be safe to use -moz-margin-end here; though as it's prefixed and this is standalone code, you may want to add other rules for other targeted platforms.

You can discuss this with :mdeboer who seems to have lots of insights about this.

@@ +57,5 @@
> +     */
> +    margin-right: 16px;
> +  }
> +
> +.conversation-toolbar-btn-box {

nit: this is not really a descriptive name… I'd rather see a child selector here, like .conversation-toolbar > li.

@@ +65,5 @@
> +  .standalone .conversation-toolbar-btn-box {
> +    /* overwrite the style for standalone
> +     * because we reuse the same component */
> +    border: none;
> +  }

I'd rather see this defined the other way around, just defining the border for the desktop version.

@@ +85,5 @@
> +}
> +
> +  .standalone-conversation-toolbar-media-btn:hover {
> +    background-color: rgba(255,255,255,.35);
> +  }

nit: I've been told in the past that indenting CSS rules was considered "bad practice" at mozilla (never found any pointer about it though); personally, I like it a lot. Your call.

@@ +282,1 @@
>  }

Still unsure about this way of "spacing" elements. I'd rather see style overriding for elements we want space added to.

::: browser/components/loop/content/shared/css/panel.css
@@ +41,4 @@
>    border-radius: 2px;
>    outline: 0;
> +  height: 26px;
> +  width: calc(100% - 28px);

Please add a comment explaining where these 28px come from (I think it's because of the 14px top & bottom margins added by a previous rule).

@@ +94,5 @@
>  }
>  
>  .dnd-menu {
>    position: absolute;
> +  top: -28px;

Please add a comment explaining where these 28px come from.

::: browser/components/loop/standalone/content/css/webapp.css
@@ +29,5 @@
>  
>  /*
>   * Top/Bottom spacing
>   **/
> +.standalone-footer {

how about .standalone footer?

@@ +104,5 @@
>    background-repeat: no-repeat;
>  }
>  
> +.standalone-header-title,
> +.standalone-call-btn-label {

I keep thinking we should scope standalone-specific elements using composite selectors, like .standalone .call-btn-label.

@@ +194,5 @@
> +/*
> + * Left / Right padding elements
> + * used to center components
> + * */
> +.flex-padding-1 {

This really introduces non-semantic class names; I'd rather see these rules defined for semantically identified elements.

::: browser/components/loop/standalone/content/js/webapp.jsx
@@ +119,5 @@
>  
>    var ConversationFooter = React.createClass({
>      render: function() {
>        return (
> +        <div className="standalone-footer container-box">

This probably wants to be a <footer>.

@@ +232,1 @@
>                                loop.shared.utils.getTargetPlatform();

This is no longer required as we're now setting the OS class in init().

::: browser/components/loop/test/desktop-local/conversation_test.js
@@ +575,5 @@
> +        model.trigger.withArgs("accept");
> +        TestUtils.Simulate.click(buttonAccept);
> +
> +        /* Setting a model property triggers 2 events */
> +        sinon.assert.calledOnce(model.trigger.withArgs("accept"));

The comment above seems to contradict this assertion…

::: browser/components/loop/ui/ui-showcase.css
@@ +27,2 @@
>    margin-right: .5em;
> +  padding: .4rem;

Mixing em and rem seems impractical; we should probably use rem everywhere in this file.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +17,5 @@
>    var IncomingCallView = loop.conversation.IncomingCallView;
>  
>    // 2. Standalone webapp
> +  var CallUrlExpiredView    = loop.webapp.CallUrlExpiredView;
> +  var StartConversationView = loop.webapp.StartConversationView;

Note: as of React 0.11, you can use namespaced component tags; <loop.webapp.CallurlExpiredView /> should just work. Not sure that would increase legibility here though.

@@ +204,5 @@
>    });
>  
>    window.addEventListener("DOMContentLoaded", function() {
> +    var body = document.body;
> +    body.className = loop.shared.utils.getTargetPlatform();

Hmm at some point we'll probably want to "emulate" rendering against different target platforms… I don't know, but if we do, we want a bug filed :)

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

After discussion with Niko & Andrei yesterday and today, the previous review doesn't really make sense for this context.  We've since come to agreement that the patch is in good enough shape to land as-is.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Part 1 of visual uplift, patch by loop frontend-ers

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Part 1 of visual uplift, patch by loop frontend-ers, now with more fixes

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Part 1 of visual uplift, patch by loop frontend-ers

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8479462 [details] [diff] [review]
Part 1 of visual uplift, patch by loop frontend-ers

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

This version has fx-team merge changes, as well as fixing bugs caught by the automated tests and the UI showcase.

To be clear, the patch here was effectively written on a branch by :NiKo`, Mike de Boer, :andreio, and me.  The code has all been reviewed, for the most part in small chunks as we went.  I'll take responsibility for the overall review here.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/8ba9dc8b44c7

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: [checked-in] Part 1 of visual uplift, patch by loop frontend-ers

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8480012 [details] [diff] [review]
[checked-in] Part 1 of visual uplift, patch by loop frontend-ers

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

Carrying forward r=dmose; this is what was actually landed.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ba9dc8b44c7

Comment 24

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

Attachment: Part 2 of visual uplift

Comment 25

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

This patch implements breakpoints @ 640px for the conversation view in standalone view.
It adds the ability to view the breakpoint in  the UI showcase
** wrap the component in a .breakpoint & add in an empty iframe in the same container and the script will take care of the rest
It adds active state components to the UI.
** add data-trigger-click attribute with the className of the DOM node that needs to be triggered. It uses React Simulate to trigger the active state.
It refactors part of the CSS to some extend (using no more that 1 descended selector)

* Not sure if this patch needs a spinoff bug

Comment 26

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

Attachment: Part 2 of visual uplift

Comment 27

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

Updates:
* fix standalone window for < 640px, resizing displayed a weird ratio
* add default avatar for local and remote streams bug 1029436
* fix font family in standalone (not using platform specific fonts)
* adds alerts & fixes close button

* resizing issue in desktop version for the detached window is tracked in bug 1059986

Comment 28

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8480995 [details] [diff] [review]
Part 2 of visual uplift

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

Tested the patch, it improves the situation quite a bunch :)

Comparing standalone specs with the current result though, I noticed a few differences when it comes to responsive (see http://cl.ly/image/3k1M030x0G31 and http://cl.ly/image/2X2I1z172o1J); I don't have a sense how much closely we want to follow the spec, but I doubt this would pass UX review as is. Will there be a part 3 addressing the remaining bits?

I'll keep reviewing this a bit later, because you know, bedtime :)

::: browser/components/loop/content/shared/css/conversation.css
@@ +455,5 @@
> +.local-stream.local-stream-audio,
> +.standalone .OT_subscriber .OT_video-poster,
> +.fx-embedded .OT_video-container .OT_video-poster,
> +.local-stream-audio .OT_publisher .OT_video-poster {
> +  background-image: url("../img/audio-call-avatar.svg");

For some reason this is never displayed to me, even after rebuilding…

@@ +460,5 @@
> +  background-repeat: no-repeat;
> +  background-color: #4BA6E7;
> +  background-size: contain;
> +  background-position: center;
> +}

Please add a comment that this is fragile as it could break anytime the SDK updates the generate class names it uses.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +129,5 @@
>            </Section>
>  
>            <Section name="ConversationToolbar">
>              <h3>Desktop Conversation Window</h3>
> +            <div className="conversation-window fx-embedded override-position">

I think conversation-window is obsolete and could be removed.

@@ +272,5 @@
>    });
>  
> +  /**
> +   * Simulate events and enable active state for component showcase
> +   * */

Frankly, this whole part of the code would highly benefit from a live pair review, as it's a bit difficult to grasp & follow :)

Comment 29

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8480995 [details] [diff] [review]
Part 2 of visual uplift

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

Added more comments about the patch. I think it still needs some work before landing…

::: browser/components/loop/content/shared/css/conversation.css
@@ +164,2 @@
>    display: inline-block;
> +  padding-bottom: 75%;

Please keep the previous comment ("/* XXX forced 4:3 ratio, see bug 1020445 */")

@@ +166,2 @@
>    width: 100%;
> +  height: 200px;

Please explain where this value comes from. Also, I have a hunch this defeats the purpose of using padding-bottom to preserve initial aspect ratio… Am I wrong?

@@ +460,5 @@
> +  background-repeat: no-repeat;
> +  background-color: #4BA6E7;
> +  background-size: contain;
> +  background-position: center;
> +}

Please add a comment that this is fragile as it could break anytime the SDK updates the generate class names it uses.

@@ +478,5 @@
> +    bottom: 15px;
> +    width: 20%;
> +    height: 20%;
> +    max-width: 300px;
> +    max-height: 200px;

So that would be a 3:2 aspect ratio. Is it on purpose? What happens with a standard 4:3 aspect ratio?

@@ +492,5 @@
> +    top: 0px;
> +    right: 0px;
> +    bottom: 0px;
> +    left: 0px;
> +    padding-bottom: 75%;

Please preserve the comment about this being used to preserve a 4:3 aspect ratio.

@@ +537,5 @@
> +  }
> +
> +  .standalone .video_wrapper.remote_wrapper {
> +    /* Because of OT markup we need to set a high flex value */
> +    flex: 10;

I don't get why 100% is not enough here. Hints?

::: browser/components/loop/ui/ui-showcase.jsx
@@ +129,5 @@
>            </Section>
>  
>            <Section name="ConversationToolbar">
>              <h3>Desktop Conversation Window</h3>
> +            <div className="conversation-window fx-embedded override-position">

I think conversation-window is obsolete and could be removed.

@@ +179,5 @@
>            <Section name="ConversationView">
>  
>              <Example summary="Desktop conversation window" dashed="true"
>                       style={{width: "260px", height: "265px"}}>
> +              <div className="conversation-window fx-embedded">

conversation-window is obsolete and can be removed

@@ +271,5 @@
>      }
>    });
>  
> +  /**
> +   * Simulate events and enable active state for component showcase

Please describe more precisely what purpose this serves: 
- what type of events this simulates
- what is "active state"
- why it's needed

This also lacks developer documentation about usage. Typically, a newcomer should know about how to add more breakpoints without having to decypher the whole code.

@@ +274,5 @@
> +  /**
> +   * Simulate events and enable active state for component showcase
> +   * */
> +  function _triggerActiveComponents() {
> +    var components = document.querySelectorAll('[data-trigger-click]');

This needs explanation; especially what this data attribute is supposed to targetn and why.

@@ +276,5 @@
> +   * */
> +  function _triggerActiveComponents() {
> +    var components = document.querySelectorAll('[data-trigger-click]');
> +    [].forEach.call(components, function(comp) {
> +      var className = comp.dataset.triggerClick;

Add a comment about data-trigger-click attr being mapped to comp.dataset.triggerClick.

@@ +277,5 @@
> +  function _triggerActiveComponents() {
> +    var components = document.querySelectorAll('[data-trigger-click]');
> +    [].forEach.call(components, function(comp) {
> +      var className = comp.dataset.triggerClick;
> +      var triggerClick = simulateClick.bind(null, comp);

Naming this function "triggerClick" is confusing; "clicker" is probably just fine.

@@ +332,5 @@
>      body.className = loop.shared.utils.getTargetPlatform();
> +
> +    /* XXX we should properly mock the component dependencies
> +     * it would remove the need for a try/catch block and would
> +     * reveal meaningful debug messages in the console

Definitely. Part of this work has been started in other patches, which have possibly landed since. Please check.

Comment 30

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Clearing needinfo.

Comment 31

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

I got the aspect ratio to match the spec [0] is standalone with breakpoint triggered [1] is detached desktop side. [2] all together

The problem I'm having now is that on resize the aspect gets busted (I think OT uses js to compute dimensions and it does so in a way that is not convenient for our usecase) I was able to solve this by adding an event listener for resize and then setting the element width or height (depending on the stream) to 100%

Not sure if we want this though. Resizing windows seems like an edge case (especially for < 640px). Right now I've added a listener for orientation change (when it is actually needed). I could do the same and target resizes. Thoughts ?

[0] http://imgur.com/ht6uhQf.png
[1] http://imgur.com/VKBBPY6.png
[2] http://i.imgur.com/4Y3Yrre.png

Comment 32

[Darrin Henein [:darrin]]

The ratio in [0] looks off to me, can we get the bottom/self view to take up a bit more space? Should be between 2:1 and 1:1 somewhere...

Comment 33

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Andrei Oprea [:andreio] from comment #31)
> Resizing windows seems like an edge case (especially for < 640px)

I think we want this for mobile users (other than FxOS, which has its own app)… NI :RT here: is UX for non FxOS users high priority? If it's just a nice to have, we probably shouldn't bother too much for now.

(In reply to Darrin Henein [:darrin] from comment #32)
> The ratio in [0] looks off to me, can we get the bottom/self view to take up
> a bit more space? Should be between 2:1 and 1:1 somewhere...

Agreed with :darrin. 3:2 might just work fine here.

Comment 34

[Romain Testard [:RT]]

(In reply to Nicolas Perriault (:NiKo`) from comment #33)
> (In reply to Andrei Oprea [:andreio] from comment #31)
> > Resizing windows seems like an edge case (especially for < 640px)
> 
> I think we want this for mobile users (other than FxOS, which has its own
> app)… NI :RT here: is UX for non FxOS users high priority? If it's just a
> nice to have, we probably shouldn't bother too much for now.
It's a nice to have only. 
We should have a mobile optimized UX for link clickers after 34.
> 
> (In reply to Darrin Henein [:darrin] from comment #32)
> > The ratio in [0] looks off to me, can we get the bottom/self view to take up
> > a bit more space? Should be between 2:1 and 1:1 somewhere...
> 
> Agreed with :darrin. 3:2 might just work fine here.

Comment 35

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

Attachment: Part 2 of visual uplift

Comment 36

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

Known issue is that active components (components that have extra UI elements hidden until a user action, like a dropdown) will toggle back to being hidden when you click anywhere on the page (it is expected behavior, we could not find an easy way to prevent this in the UI showcase though).

Comment 37

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

Attachment: Part 2 of visual uplift

Comment 38

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8483640 [details] [diff] [review]
Part 2 of visual uplift

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

This looks very good. I'll take over the patch to address the remaining bits, so we can land this asap.

r=me conditional with me attaching a new patch with comments addressed.

::: browser/components/loop/content/shared/js/views.jsx
@@ +258,5 @@
> +      /**
> +       * OT inserts inline styles into the markup. Using a listener for
> +       * resize events helps us trigger a full width/height on the element
> +       * so that they update to the correct dimensions.
> +       * */

This file has not been compiled (see the differences with views.js)

@@ +266,5 @@
> +
> +    updateVideoContainer: function() {
> +      var localStreamParent = document.querySelector('.local .OT_publisher');
> +      var remoteStreamParent = document.querySelector('.remote .OT_subscriber');
> +      localStreamParent.style.width = "100%";

This fails if the selector is no more available — most likely because already removed by the SDK.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +121,5 @@
>  
>            <Section name="IncomingCallView">
>              <Example summary="Default" dashed="true" style={{width: "280px"}}>
> +              <div className="fx-embedded">
> +                <IncomingCallView />

This needs a model instance passed in.

@@ +129,5 @@
> +
> +          <Section name="IncomingCallView-ActiveState">
> +            <Example summary="Default" dashed="true" style={{width: "280px"}}>
> +              <div className="fx-embedded" >
> +                <IncomingCallView showDeclineMenu={true} />

This needs a model instance passed in.

@@ +186,5 @@
>                <div className="standalone">
>                  <StartConversationView model={mockConversationModel}
>                                         client={mockClient}
>                                         notifier={mockNotifier} />
> +                                       showCallOptionsMenu={true} />

This double self-closing tag breaks.

@@ +241,5 @@
> +                   data-breakpoint-height="780px">
> +                <div className="standalone">
> +                  <ConversationView video={{enabled: true}}
> +                                    audio={{enabled: true}}
> +                                    model={mockConversationModel} />

This needs an sdk prop passed in.

@@ +250,5 @@
> +
> +          <Section name="ConversationView-LocalAudio">
> +            <Example summary="Local stream is audio only">
> +              <div className="standalone">
> +                <ConversationView video={{enabled: false}} audio={{enabled: true}}

This needs an sdk prop passed in.

@@ +355,5 @@
> +    try {
> +      React.renderComponent(<App />, body);
> +    } catch (e) {
> +      console.log(e);
> +    }

We need to pass the proper mock objects wherever applicable instead of doing this.

Comment 39

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: [checked in] Part 2 of visual uplift

Updated patch with review comments addressed.

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 8484089 [details] [diff] [review]
[checked in] Part 2 of visual uplift

https://hg.mozilla.org/integration/fx-team/rev/0013ab838b41

(probably should have put a slightly better commit message on it like "visual uplift for loop").

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0013ab838b41

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

We've done as much of this as is going to happen in giant chunks, so resolving this bug as fixed.

Comment 43

[u279076]

Firefox Nightly 35.0a1's UI looks just like in the mockups.
Firefox Aurora 34.0a2's UI looks mostly like in the mockups -- the only difference being the chat panel's titlebar has a grey background instead of black. Is this expected?

Comment 44

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

No idea. NI :shell and :mreavy here.

Comment 45

[Maire Reavy [:mreavy]]

This is part of the visual uplift which we will be uplifting to Fx34 next week.

Comment 46

[u279076]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #45)
> This is part of the visual uplift which we will be uplifting to Fx34 next
> week.

What's the bug number tracking this? I'd like it to be added to the dependency tree so we can show that verification is currently blocked.

Comment 47

[Maire Reavy [:mreavy]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #46)
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #45)
> > This is part of the visual uplift which we will be uplifting to Fx34 next
> > week.
> 
> What's the bug number tracking this? I'd like it to be added to the
> dependency tree so we can show that verification is currently blocked.

This is the bug to track.  We were going to spread this work out over more than one bug, but all the work was done here.   The "Part 1" patch on this bug is in Fx34 already (which is why the UI there looks mostly like the mock ups).  We need to uplift the "Part 2" patch so that Fx34 matches Fx35, and we'll do that next week.

Comment 48

[u279076]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #47)
> We need to uplift the "Part 2" patch so that Fx34 matches Fx35, and
> we'll do that next week.

Is that happening in *this* bug or some other bug? If it's happening in this bug I recommend we set the Target Milestone as mozilla35 and set the status flags to 35:fixed, 34:affected until all the work is completely uplifted.

Comment 49

[Maire Reavy [:mreavy]]

You're right.  Marking the bug now would be clearer.  No point in waiting on that, and we'll ask for aurora approval on Part 2 next week.

Comment 50

[u279076]

Thanks Maire. I'm marking this verified fixed for Firefox 35 based on comment 43.

Comment 51

[u279076]

Tracking to verify prior to uplift. Paul, can you please test this with http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9eb2cbac352 across all platforms?

Comment 52

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #51)
> Tracking to verify prior to uplift. Paul, can you please test this with
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> f9eb2cbac352 across all platforms?

Sorry, forgot the needinfo.

Comment 53

[Paul Silaghi, QA [:pauly]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #51)
> Tracking to verify prior to uplift. Paul, can you please test this with
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-
> f9eb2cbac352 across all platforms?
Verified fixed Win 7, OS X 10.9.5, Ubuntu 13.04

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8484089 [details] [diff] [review]
[checked in] Part 2 of visual uplift

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 55

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17628eb25daf

Comment 56

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8484089 [details] [diff] [review]
[checked in] Part 2 of visual uplift

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 57

[u279076]

I've seen this in Aurora testing over the last few days. Marking this verified fixed for Firefox 34.