Closed Bug 1209686 Opened 4 years ago Closed 4 years ago

Remove the header of Loop's standalone page

Categories

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

defect
Points:
5

Tracking

(firefox44 fixed)

RESOLVED FIXED
mozilla44
Iteration:
44.2 - Oct 19
Tracking Status
firefox44 --- fixed

People

(Reporter: standard8, Assigned: dcritchley)

References

Details

User Story

Acceptance criteria:

- No Header on the standalone UI
- Right and left 10px margins have been removed (no margins around the media layout element)
- The Firefox Hello logo is located in the top-left hand corner of the focus stream (normally remote video, but window/tab share video if being shown).
- The Help icon is shown at the top-right hand corner of the focus stream
- The Firefox Hello looks fine or disappears(?) at narrow screen widths.

Additional Dev notes:

- Look for the .standalone-room-wrapper > .media-layout classes in conversation.css, this should be able to be removed now.
- Remember to remove any redundant css classes.

Attachments

(4 files, 4 obsolete files)

As part of the ongoing work, we should remove the header of the standalone. Suggest this work happens after bug 1209632.

See user story for details.
Blocks: 1202371
Rank: 21
Assignee: nobody → david.critchley
Priority: P1 → P2
Pau, would you be able to make a mockup showing what the current link clicker should look like without the header row? This is just prepping the layout for the new visual refresh stuff.

Maybe you can work off of the mockup with the removed footer found here: https://bug1209632.bmoattachments.org/attachment.cgi?id=8668570
Flags: needinfo?(b.pmm)
Sevaan, I was assuming we'd go for something similar to the existing UX-refresh layout?
Flags: needinfo?(sfranks)
Yes, let's do that. Dcritch wanted to know how it should look on the old layout. David, do you need this mockup, or you can work off the link clicker layouts Vicky made?
Flags: needinfo?(sfranks)
I can work off the link clicker layouts.
No need anymore, Pau.
Flags: needinfo?(b.pmm)
Note: Vicky's mock-ups use the Blue Face logo - we should continue using the Firefox logo for now - I believe there's something that outside of Firefox we need the FF logo, but inside we can use Hello's. However, lets leave sorting that out to a separate bug.
IMHO, Mark, if we want to make sure our brand, which is Firefox Hello, is recognizable inside and outside Firefox environment, we need to put Hello's logo in first place. If we also want to let users know it's a Firefox product, then we should also include FF Logo but never in the same level of importance since both logos should never compete. But placing only the FF Logo in a product users don't know what it is makes no sense at all.
(In reply to Pau Masiá [:Pau] from comment #7)
> IMHO, Mark, if we want to make sure our brand, which is Firefox Hello, is
> recognizable inside and outside Firefox environment, we need to put Hello's
> logo in first place. If we also want to let users know it's a Firefox
> product, then we should also include FF Logo but never in the same level of
> importance since both logos should never compete. But placing only the FF
> Logo in a product users don't know what it is makes no sense at all.

This was out of a discussion from our Marketing folks a long time ago, and its why we use the Firefox logo on standalone today. I'm going to needinfo to Romain to check if he knows if that's changed or not.
Flags: needinfo?(rtestard)
Just re-confirmed with Fabio - we want the Firefox Hello logo per the UX.
Flags: needinfo?(rtestard)
Just to clarify what I'm doing as part of this bug. Am I changing the layout only? Or are there new elements being added here?
Attached image Mock Up of layout
(In reply to David Critchley (:dcritch) from comment #10)
> Just to clarify what I'm doing as part of this bug. Am I changing the layout
> only? Or are there new elements being added here?

Same as the user story. However, for the "Firefox Hello" logo, you'll need to use hello_logo.svg rather than the firefox logo + text.

Also, no margins at all around the screen - as per the mock-up I'm attaching.
Size of Logo (Currently 128px x 24px) and Help icon (Currently 20px x 20px)?
Should Help icon be visible in all states?
Help icon colors Gray on white or white on gray? The mocks show gray on white, but the current icon we have is white on gray.
Flags: needinfo?(sfranks)
Attachment #8670499 - Flags: ui-review?(sfranks)
Remove Standalone header from Loop
Attachment #8670532 - Flags: review?(edilee)
Comment on attachment 8670532 [details] [diff] [review]
Attachment to Bug 1209686 - Remove the header of Loop's standalone page

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

I'd just like to add some architecture notes to this. I think it would be nicer to add a new component as well as GeneralSupportURL. The component would be something like StandaloneOverlayWrapper, and would encompass the logo, support url and also the mozilla-logo from the footer. Since these are all absolutely positioned it won't matter if we include them in one wrapper.

The css can then become something like .standalone-overlay-wrapper > .hello-logo which is obviously a bit more self-contained.

We should also move the standalone specific css into the webapp.css file - the previous header stuff shouldn't have really been in conversation.css as it doesn't apply to the conversation window.
(In reply to David Critchley (:dcritch) from comment #13)

> Size of Logo (Currently 128px x 24px) and Help icon (Currently 20px x 20px)?

Let's make the Help icon 16x16

> Should Help icon be visible in all states?

No, it can disappear after Join, I think.

> Help icon colors Gray on white or white on gray? The mocks show gray on
> white, but the current icon we have is white on gray.

What you have on the screenshot looks fine for this dark background, but it may need to be inversed when we lighten the background.
Flags: needinfo?(sfranks)
Attachment #8670499 - Flags: ui-review?(sfranks) → ui-review+
Attached image disregard this (obsolete) —
Attachment #8671030 - Flags: ui-review?(sfranks)
Comment on attachment 8671030 [details]
disregard this

Added to wrong bug
Attachment #8671030 - Attachment description: Screenshot of Tabs Removed → disregard this
Attachment #8671030 - Attachment is obsolete: true
Attachment #8671030 - Flags: ui-review?(sfranks)
Remove Standalone header from Loop
Attachment #8670532 - Attachment is obsolete: true
Attachment #8670532 - Flags: review?(edilee)
Attachment #8671700 - Flags: review?(dmose)
Comment on attachment 8671700 [details] [diff] [review]
Attachment to Bug 1209686 - Remove the header of Loop's standalone page

>+++ b/browser/components/loop/content/shared/css/conversation.css
>@@ -1119,17 +1063,17 @@ body[platform="win"] .share-service-dropdown.overflow > .dropdown-menu-item {
>   .media-wrapper > .focus-stream > .local ~ .conversation-toolbar {
>-    max-width: calc(75% - 22px);
>+    max-width: calc(75%);
No need for calc here.


>+++ b/browser/components/loop/content/shared/js/views.jsx
>@@ -1112,25 +1112,25 @@ loop.shared.views = (function(_, mozL10n) {
>             <div className={remoteStreamClasses}>
>+              { this.props.displayScreenShare ? null : this.props.children }
>             <div className={screenShareStreamClasses}>
>+              { this.props.displayScreenShare ? this.props.children : null }
Is this change to make the overlay be hidden after joining? If so, this seems a bit heavyhanded in that there's other children of the MediaLayoutView, e.g., StandaloneRoomInfoArea and ConversationToolbar that would get hidden. Probably better to have StandaloneOverlayWrapper know how to hide itself.

>+++ b/browser/components/loop/standalone/content/css/webapp.css
>@@ -41,57 +41,95 @@ body,
>+/* Standalone Overlay wrapper */
>+.standalone-overlay-wrapper {
>+  position: absolute;
>+  width: 100%;
>+  height: 100%;
This overlay sits on top of other stuff, so things like the Join button aren't clickable. You probably want pointer-events: none and reenable clicking with auto on the desired elements.

>+.standalone-overlay-wrapper .icon-help {
>+  display: inline-block;
I'm not sure what pattern we want to follow, but I thought <i> was trying to be used for inline background images while <div> was for a block background image. Although here it's an inline-block... I see both <i> and <div> as well as <img> being used where the first 2 might be preferrable in that the images are specified in css instead of js.. ?

>+  .standalone-overlay-wrapper > .hello-logo {
>+    display:none;
nit: space after :

>+++ b/browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
>+  var StandaloneOverlayWrapper = React.createClass({
>   var StandaloneMozLogo = React.createClass({
>     render: function() {
>       return (
>         <div className="standalone-moz-logo" />
Standard8 requested this StandaloneOverlayWrapper in addition to GeneralSupportURL, but now that StandaloneMozLogo is a logic-less <div>, I'm not sure if it's worthwhile to have a separate StandaloneMozLogo instead of just inlining it into StandaloneOverlayWrapper.
(In reply to Ed Lee :Mardak from comment #20)
> >+++ b/browser/components/loop/content/shared/js/views.jsx
> >@@ -1112,25 +1112,25 @@ loop.shared.views = (function(_, mozL10n) {
> >             <div className={remoteStreamClasses}>
> >+              { this.props.displayScreenShare ? null : this.props.children }
> >             <div className={screenShareStreamClasses}>
> >+              { this.props.displayScreenShare ? this.props.children : null }
> Is this change to make the overlay be hidden after joining? If so, this
> seems a bit heavyhanded in that there's other children of the
> MediaLayoutView, e.g., StandaloneRoomInfoArea and ConversationToolbar that
> would get hidden. Probably better to have StandaloneOverlayWrapper know how
> to hide itself.

These alternate where the children are defined between the remote stream & screen share - i.e. the focus stream. This means they'll always get displayed relative to the main part of the display, rather than being shifted into the sidebar when screen shares are displayed.
(In reply to Mark Banner (:standard8) from comment #21)
> These alternate where the children are defined between the remote stream & screen share
Ah that makes more sense. Are we doing the disappearing sevaan mentioned in comment 16 in a separate bug? Although looking more closely at that comment, the disappearing would only be for the help icon.
In discussion with Sevaan, decided that all elements of the StandaloneOverlay will be displayed. Help button will no longer disappear when joining.
Remove Standalone header from Loop
Attachment #8671700 - Attachment is obsolete: true
Attachment #8671700 - Flags: review?(dmose)
Attachment #8672153 - Flags: review?(edilee)
Comment on attachment 8672153 [details] [diff] [review]
Attachment to Bug 1209686 - Remove the header of Loop's standalone page

>+++ b/browser/components/loop/standalone/content/css/webapp.css
>+/* Standalone Overlay wrapper */
>+.standalone-overlay-wrapper {
>+  position: absolute;
>+  width: 100%;
>+  height: 100%;
>   left: 0;
>+  top: 0;
>+  pointer-events: none;
Instead of setting height/width 100%, you can set right/bottom to 0 and give this a margin of 1.2rem. This will make it so the 3 items you place inside can be placed at top/right/bottom/left: 0 instead of 1.2rem each.

>+.standalone-overlay-wrapper > .hello-logo {
>+  position: absolute;
>+  left: 1.2rem;
>+  right: auto;
>+  top: 1.2rem;
As per comment above, the left/top values can be 0 instead. No need for explicit right: auto. (Same comment for other similar styles.)

>+  width: 128px;
>+  height: 24px;
>+  background-image: url("../shared/img/hello_logo.svg");
>+  background-repeat: no-repeat;
>+  background-position: left top;
>+  background-size: 100%;
The resized hello logo ends up being 128x21, so if you set the height appropriately, you won't need no-repeat or left top background positioning.

>+.standalone-overlay-wrapper .icon-help {
>+  display: inline-block;
If we make the icon a div, no need to override the default display: block of the div. See later comment.

>+html[dir="rtl"] .standalone-overlay-wrapper> .standalone-moz-logo {
nit: space before >

>+++ b/browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
>+  var StandaloneOverlayWrapper = React.createClass({
>     render: function() {
>+        <div className="standalone-overlay-wrapper">
>+          <i className="hello-logo"></i>
>+          <GeneralSupportURL dispatcher={this.props.dispatcher} />
>+          <div className="standalone-moz-logo" />
>+  var GeneralSupportURL = React.createClass({
>+    render: function() {
>+        <a className="general-support-url"
>+          <i className="icon icon-help"></i>
Standard8, I would just make the two <i> as <div> like the standalone-moz-logo... Does that seem reasonable? (<a><div></a> is okay in html5! ;))
Attachment #8672153 - Flags: review?(edilee)
Attachment #8672153 - Flags: review+
Attachment #8672153 - Flags: feedback?(standard8)
Comment on attachment 8672153 [details] [diff] [review]
Attachment to Bug 1209686 - Remove the header of Loop's standalone page

(In reply to Ed Lee :Mardak from comment #25)
> >+++ b/browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
> >+  var StandaloneOverlayWrapper = React.createClass({
> >     render: function() {
> >+        <div className="standalone-overlay-wrapper">
> >+          <i className="hello-logo"></i>
> >+          <GeneralSupportURL dispatcher={this.props.dispatcher} />
> >+          <div className="standalone-moz-logo" />
> >+  var GeneralSupportURL = React.createClass({
> >+    render: function() {
> >+        <a className="general-support-url"
> >+          <i className="icon icon-help"></i>
> Standard8, I would just make the two <i> as <div> like the
> standalone-moz-logo... Does that seem reasonable? (<a><div></a> is okay in
> html5! ;))

It seems to be reasonable to use a div. I had a look around, and the best I could get was "most modern browsers" support it. So I think we should be ok.
Attachment #8672153 - Flags: feedback?(standard8)
Remove Standalone header from Loop
Attachment #8672153 - Attachment is obsolete: true
Attachment #8673268 - Flags: review?(edilee)
Comment on attachment 8673268 [details] [diff] [review]
Attachment to Bug 1209686 - Remove the header of Loop's standalone page

No need to request ?review after getting r+ unless you think there's major changes.

>+++ b/browser/components/loop/standalone/content/js/standaloneRoomViews.js
>+        React.createElement("div", {className: "standalone-overlay-wrapper"}, 
>+          React.createElement("div", {className: "hello-logo"}), 
>+          React.createElement(GeneralSupportURL, {dispatcher: this.props.dispatcher}), 
>+          React.createElement("div", {className: "standalone-moz-logo"})

>+++ b/browser/components/loop/standalone/content/js/standaloneRoomViews.jsx
>+        <div className="standalone-overlay-wrapper">
>+          <div className="hello-logo"></div>
>+          <GeneralSupportURL dispatcher={this.props.dispatcher} />
>+          <div className="standalone-moz-logo" />
>+        </div>

Good to know that React will handle <div></div> and <div/> just fine.
Attachment #8673268 - Flags: review?(edilee) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/635af6c39f9e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Iteration: --- → 44.2 - Oct 19
You need to log in before you can comment on or make changes to this bug.