1209686 - Remove the header of Loop's standalone page

Status: RESOLVED FIXED

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

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Sevaan Franks [:sevaan]]

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

Comment 2

[Mark Banner (:standard8)]

Sevaan, I was assuming we'd go for something similar to the existing UX-refresh layout?

Comment 3

[Sevaan Franks [:sevaan]]

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?

Comment 4

[David Critchley (:dcritch)]

I can work off the link clicker layouts.

Comment 5

[Sevaan Franks [:sevaan]]

No need anymore, Pau.

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Pau Masiá [:Pau]]

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.

Comment 8

[Mark Banner (:standard8)]

(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.

Comment 9

[Romain Testard [:RT]]

Just re-confirmed with Fabio - we want the Firefox Hello logo per the UX.

Comment 10

[David Critchley (:dcritch)]

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?

Comment 11

[Mark Banner (:standard8)]

Attachment: 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.

Comment 12

[Ed Lee :Mardak]

Attachment: link clicker desktop wip (?)

Comment 13

[David Critchley (:dcritch)]

Attachment: Screenshot of Changes for Review

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.

Comment 14

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1209686 - Remove the header of Loop's standalone page

Remove Standalone header from Loop

Comment 15

[Mark Banner (:standard8)]

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.

Comment 16

[Sevaan Franks [:sevaan]]

(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.

Comment 17

[David Critchley (:dcritch)]

Attachment: disregard this

Comment 18

[David Critchley (:dcritch)]

Comment on attachment 8671030 [details]
disregard this

Added to wrong bug

Comment 19

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1209686 - Remove the header of Loop's standalone page

Remove Standalone header from Loop

Comment 20

[Ed Lee :Mardak]

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.

Comment 21

[Mark Banner (:standard8)]

(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.

Comment 22

[Ed Lee :Mardak]

(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.

Comment 23

[David Critchley (:dcritch)]

In discussion with Sevaan, decided that all elements of the StandaloneOverlay will be displayed. Help button will no longer disappear when joining.

Comment 24

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1209686 - Remove the header of Loop's standalone page

Remove Standalone header from Loop

Comment 25

[Ed Lee :Mardak]

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! ;))

Comment 26

[Mark Banner (: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.

Comment 27

[David Critchley (:dcritch)]

Attachment: Attachment to Bug 1209686 - Remove the header of Loop's standalone page

Remove Standalone header from Loop

Comment 28

[Ed Lee :Mardak]

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.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/635af6c39f9e

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/635af6c39f9e