Closed Bug 1205684 Opened 4 years ago Closed 4 years ago

Video Window height pushes down context and covers a part of it

Categories

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

defect

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.3 - Nov 2
Tracking Status
firefox44 --- fixed

People

(Reporter: Pau, Assigned: mancas)

References

Details

(Whiteboard: [visual refresh defect][conversation])

Attachments

(10 files, 5 obsolete files)

759.84 KB, image/png
Details
117.83 KB, image/png
Details
60.78 KB, image/png
Details
116.20 KB, image/png
Details
185.19 KB, image/png
Details
387.06 KB, image/png
Details
213.88 KB, image/png
Details
21.71 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
21.78 KB, patch
Details | Diff | Splinter Review
21.24 KB, patch
Details | Diff | Splinter Review
Fix video height to 45% of total height.
Rank: 20
Priority: -- → P2
Whiteboard: [visual refresh defect][conversation]
See Also: → 1200693
I have to admit, 45% seems a little bit on the large side to me - especially for the narrower popped out/standalone views that I'll attach in a moment. Hence NI to Sevaan/Pau.

There's also a regression with when we display the context - we'll get pillarboxing on the local video as per this screenshot.

The standalone doesn't look quite right either when in the narrow view - the text chat area doesn't seem to be taking up the 45% height fully.
Flags: needinfo?(sfranks)
Flags: needinfo?(b.pmm)
Attached image Standalone black space
Oh, the other thing I forgot to mention, is if we're not uplifting this to 43, then maybe the pillarbox regression doesn't matter - depending on what we're displaying on the link generator side when we enter the room.
Comment on attachment 8670769 [details] [diff] [review]
Fix: the height of context and chat is set to 45% of the total space

I think there's visual regressions here, and we need answers from UX before we can move forward.
Attachment #8670769 - Flags: review?(standard8) → review-
Attached image Convo Window Fixed
We need to make sure the video aspect ratio is 4:3 so as the conversation window is 350px wide, we need to make video 263px height. The rest of the real state is used for placing content (context tile + chat bubbles) as shown in this snap shot. Scroll is enabled only when the first chat bubble appears.
Flags: needinfo?(b.pmm)
Standalone needs to be reviewed to make sure it doesn't crash. Manu and I are working on it.
Taking into account Pau comments, I've implemented a new patch. Could you review it Mark?

Thanks
Attachment #8670769 - Attachment is obsolete: true
Attachment #8674242 - Flags: review?(standard8)
Comment on attachment 8674242 [details] [diff] [review]
Video Window height pushes down context and covers a part of it

Short of time today, so passing to Mike
Attachment #8674242 - Flags: review?(standard8) → review?(mdeboer)
Comment on attachment 8674242 [details] [diff] [review]
Video Window height pushes down context and covers a part of it

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

I like the fact that you added another sizeMode, called 'LoopChatDisabledMessageAppended'. Makes sense.
I'd like to see a version that doesn't touch the font sizes too much.

::: browser/components/loop/content/shared/css/common.css
@@ +541,5 @@
>  .context-content > p {
>    font-weight: bold;
>    margin-bottom: .8em;
>    margin-top: 0;
> +  font-size: 13px;

I think the font sizes are a bit large... at least, that's how it looks to me. Can you post screenshots of this patch in action and request ui-review?
Why do we need these font-size adjustments at all? I was hoping that this patch would only touch window dimensions.
On top of that, we try to keep everything text-related defined with dynamic units, like 'em' and 'rem'.
Attachment #8674242 - Flags: review?(mdeboer) → review-
Flags: needinfo?(sfranks)
Attached image no chat.png (obsolete) —
Attachment #8676155 - Flags: ui-review?(b.pmm)
Attached image chat with scroll.png (obsolete) —
Attachment #8676156 - Flags: ui-review?(b.pmm)
Attached image chat enabled.png (obsolete) —
Attachment #8676166 - Flags: ui-review?(b.pmm)
Hey Mike, I worked in pair with Pau to implement this solution. That's the reason why the fonts have been adjusted, because he thought that it was necessary. I'm ok using dynamic units so let me change it and upload a new patch.

Thanks!
Flags: needinfo?(mdeboer)
Comment on attachment 8676155 [details]
no chat.png

Nice work, Manu!

And, in reply of comment 9, Mike, we had to modify fonts size because the window size changed and it got bigger so to match that change visually we had to make some tweaks.

Now, looking at it, I've noticed the "invite a friend..." text looks a bit small to me and even buttons but that's something to talk about in a follow-up bug, I guess.
Attachment #8676155 - Flags: ui-review?(b.pmm) → ui-review+
Comment on attachment 8676156 [details]
chat with scroll.png

UI-Review +, but is there a reason why the settings icon is overlapping the user's video thumbnail?
Attachment #8676156 - Flags: ui-review?(b.pmm) → ui-review+
Attachment #8676166 - Flags: ui-review?(b.pmm) → ui-review+
(In reply to Manuel Casas Barrado [:mancas] from comment #13)
> Hey Mike, I worked in pair with Pau to implement this solution. That's the
> reason why the fonts have been adjusted, because he thought that it was
> necessary. I'm ok using dynamic units so let me change it and upload a new
> patch.
> 
> Thanks!

Well, the thing is that the context area had a scrollbar when I opened the window on OSX. I don't think we want that. Also, when I scroll that area (the chat box), it shows a visual glitch.
All in all, I'd recommend against doing the font size changes here and spin _that_ off to a follow-up bug.
Flags: needinfo?(mdeboer)
Attached image chat_disabled.png
Attachment #8676155 - Attachment is obsolete: true
Attachment #8677349 - Flags: ui-review?(b.pmm)
Attached image chat enabled.png
Attachment #8676166 - Attachment is obsolete: true
Attachment #8677350 - Flags: ui-review?(b.pmm)
Attached image chat_scroll.png
Attachment #8676156 - Attachment is obsolete: true
Attachment #8677352 - Flags: ui-review?(b.pmm)
I've reverted the font size changes so we can do it in a follow-up. Unfortunately I don't have OSX to check the issue you mentioned.
Attachment #8674242 - Attachment is obsolete: true
Attachment #8677353 - Flags: review?(mdeboer)
Attachment #8677349 - Flags: ui-review?(b.pmm) → ui-review+
Comment on attachment 8677353 [details] [diff] [review]
Video Window height pushes down context and covers a part of it

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

\o/ me likey! Looks very good.
Attachment #8677353 - Flags: review?(mdeboer) → review+
Attachment #8677350 - Flags: ui-review?(b.pmm) → ui-review+
Attachment #8677352 - Flags: ui-review?(b.pmm) → ui-review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Attached patch rebasedSplinter Review
I'll check this in.
https://hg.mozilla.org/integration/fx-team/rev/79a231b4477d1811bc85a1ad1cb916a5dae401b4
Bug 1205684 - Video Window height pushes down context and covers a part of it [r=mikedeboer]
Attached patch Fix: TestsSplinter Review
Tests are working now.
Checkin-needed: "Fix: Tests"
Flags: needinfo?(b.mcb)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1568548c87b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.3 - Nov 2
Depends on: 1227109
You need to log in before you can comment on or make changes to this bug.