Closed
Bug 1205684
Opened 9 years ago
Closed 9 years ago
Video Window height pushes down context and covers a part of it
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox44 fixed)
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
|
Pau
:
ui-review+
|
Details |
387.06 KB,
image/png
|
Pau
:
ui-review+
|
Details |
213.88 KB,
image/png
|
Pau
:
ui-review+
|
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.
Updated•9 years ago
|
Rank: 20
Priority: -- → P2
Whiteboard: [visual refresh defect][conversation]
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8670769 -
Flags: review?(standard8)
Comment 2•9 years ago
|
||
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)
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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-
Reporter | ||
Comment 5•9 years ago
|
||
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)
Reporter | ||
Comment 6•9 years ago
|
||
Standalone needs to be reviewed to make sure it doesn't crash. Manu and I are working on it.
Assignee | ||
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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 9•9 years ago
|
||
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-
Updated•9 years ago
|
Flags: needinfo?(sfranks)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8676155 -
Flags: ui-review?(b.pmm)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8676156 -
Flags: ui-review?(b.pmm)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8676166 -
Flags: ui-review?(b.pmm)
Assignee | ||
Comment 13•9 years ago
|
||
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)
Reporter | ||
Comment 14•9 years ago
|
||
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+
Reporter | ||
Comment 15•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Attachment #8676166 -
Flags: ui-review?(b.pmm) → ui-review+
Comment 16•9 years ago
|
||
(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)
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8676155 -
Attachment is obsolete: true
Attachment #8677349 -
Flags: ui-review?(b.pmm)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8676166 -
Attachment is obsolete: true
Attachment #8677350 -
Flags: ui-review?(b.pmm)
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8676156 -
Attachment is obsolete: true
Attachment #8677352 -
Flags: ui-review?(b.pmm)
Assignee | ||
Comment 20•9 years ago
|
||
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)
Reporter | ||
Updated•9 years ago
|
Attachment #8677349 -
Flags: ui-review?(b.pmm) → ui-review+
Comment 21•9 years ago
|
||
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+
Reporter | ||
Updated•9 years ago
|
Attachment #8677350 -
Flags: ui-review?(b.pmm) → ui-review+
Reporter | ||
Updated•9 years ago
|
Attachment #8677352 -
Flags: ui-review?(b.pmm) → ui-review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 23•9 years ago
|
||
I'll check this in.
Comment 24•9 years ago
|
||
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]
Backed out for Mn failures: https://treeherder.mozilla.org/logviewer.html#?job_id=5308558&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/dd5df13fccbd
Flags: needinfo?(b.mcb)
Assignee | ||
Comment 26•9 years ago
|
||
Tests are working now.
Assignee | ||
Comment 27•9 years ago
|
||
Checkin-needed: "Fix: Tests"
Flags: needinfo?(b.mcb)
Keywords: checkin-needed
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a1568548c87b
Keywords: checkin-needed
Comment 29•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1568548c87b
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•9 years ago
|
Iteration: --- → 44.3 - Nov 2
You need to log in
before you can comment on or make changes to this bug.
Description
•