1168829 - Rework link-clicker display for text chat

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

For text chat, we need to rework the standalone layout as required by the latest mock-ups (to be attached).

See user story for what needs doing.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1. Remove Loop's standalone UI old dynamic layout and context views.

Ok, these patches apply on top of the two in bug 1173909.

This first patch strips out the existing dynamic placement of the video for the standalone view. It also removes the existing StandaloneRoomContext* as they aren't going to be used any more.

I've left some of the video dimensions things in the mixin, as its unclear yet if we will need them or not to fix the remaining issues (I'll do a follow-up comment in a moment).

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

This is the main part of the work. It drops the old layout, and creates a new one, currently based around a "media-layout" div. This is flex based, with the first column forced to contain just the remote video (or screen share), and then everything else pushed into subsequent columns by the use of wrapping.

The aim of the new media-layout div was to give us a fresh "component" from which to hang our styles. I'm also expecting that we'll turn it into its own view and share it across desktop rooms & direct calls. However that'll take a bit more work, so I haven't done that yet.

There were some other styles that were altering things in the tree, so I've scoped those to the older ".conversation" class for the old call-urls, until we manage to finally deprecate those.

The ui-showcase has also been updated with another narrow-width view. The narrow width views aren't the best, but they're better than what we have now, and we can iterate more as we head towards the main refresh.

The rest of the ui-showcase views are all representative of the real thing, and were really helpful in debugging this.

Comment 3

[Mark Banner (:standard8)]

Attachment: Part 3. Fix some RTL issues associated with the new layout for Loop's text-chat.

This is a small patch on top, to fix some of the more obvious RTL issues, now that the layout of media elements is much better.

I've not touched things like the conversation toolbar - that needs a more complex patch, and it doesn't look dreadfully bad in rtl mode anyway (unlike what the rest of it looked like).

Comment 4

[Mark Banner (:standard8)]

Things that still need addressing (for other bugs!):

- Are we happy to use object-fit? (need to discuss with product/ux, will do so today).
- Using a super-short display causes bad effects.
- If we need to handle different ratios for local video other than 4:3 (we need to display all the video, and if so, we should adjust the height of the chat section to accommodate display).

Comment 5

[Mark Banner (:standard8)]

Attachment: Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

Updated a little bit of the css layout, following a ui-review from sevaan over irc - the footer & header are now 10px margins, the help icon is more vertically central, but the FF logo is still overlapped by the beta logo a little but not too much.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8623644 [details] [diff] [review]
Part 3. Fix some RTL issues associated with the new layout for Loop's text-chat.

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

::: browser/components/loop/standalone/content/css/webapp.css
@@ +353,5 @@
>  .standalone .room-conversation-wrapper .ended-conversation .feedback {
>    right: 35%;
>  }
>  
> +html[dir="rtl"] .standalone .room-conversation-wrapper .ended-conversation .feedback {

ugh, super in-efficient selector. But ok.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8623856 [details] [diff] [review]
Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

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

I believe this is all good, but I'd really like to see an effort to use fewer sibling selectors and more child-selectors. Our CSS is getting slower and slower to process and reflow.

::: browser/components/loop/content/shared/css/conversation.css
@@ +1161,5 @@
> +}
> +
> +@media screen and (max-width:640px) {
> +  .media-layout {
> +    /* XXX Doc me */

Please follow-up here ;)

@@ +1571,5 @@
>  
>  .remote-video {
>    width: 100%;
>    height: 100%;
> +  /* XXX

Care to complete this work?

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> I believe this is all good, but I'd really like to see an effort to use
> fewer sibling selectors and more child-selectors. Our CSS is getting slower
> and slower to process and reflow.

Whoops, I meant descendent selectors, not sibling selectors :-/

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

Updated the css to use child selectors. I've only touched the ones I've added. I'm hoping we'll be able to get rid of the older css soon.

Comment 10

[Mark Banner (:standard8)]

Attachment: Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

Correct the text chat layout.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8624176 [details] [diff] [review]
Part 2. Adjust Loop's Standalone UI to include text chat for conversations.

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

All done! Cool!

::: browser/components/loop/content/shared/css/conversation.css
@@ +1247,5 @@
> +    /* Screen shares have remote & local video side-by-side on narrow screens */
> +    order: 3;
> +    flex: 1 1 auto;
> +    height: 20%;
> +    /* Ensure no previously specified widths take affect, and we take up no more

nit: s/affect/effect/

@@ +1251,5 @@
> +    /* Ensure no previously specified widths take affect, and we take up no more
> +       than half the width. */
> +    width: auto;
> +    max-width: 50%;
> +    /* This cancels out the absolute positioning when its just remote video. */

nit: s/its/it's/

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ec9c9d0b9d75
https://hg.mozilla.org/integration/fx-team/rev/f160bb5abec1
https://hg.mozilla.org/integration/fx-team/rev/5442bb7741fc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec9c9d0b9d75
https://hg.mozilla.org/mozilla-central/rev/f160bb5abec1
https://hg.mozilla.org/mozilla-central/rev/5442bb7741fc

Comment 14

[Bogdan Maris, Desktop QA]

I did some exploratory testing around this fix using Firefox Developer Edition 41.0a2 across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) and I can confirm the layout implementation is according to user story (the other way around RTL builds though - expected), found no new issue so I'll mark this as verified fixed.