Skip to content
This repository has been archived by the owner on Mar 15, 2018. It is now read-only.

Firefox Accounts migration banner (bug 1052864) #657

Closed

Conversation

mstriemer
Copy link
Contributor

https://bugzilla.mozilla.org/show_bug.cgi?id=1052864

This adds custom elements for the site banner (<mkt-banner>) and login (<mkt-login>).

r? @cvan

@mstriemer
Copy link
Contributor Author

fx-accounts-banner-320x480
320x480

fx-accounts-banner-768x1024
768x1024

fx-accounts-banner-1280x600
1280x600

@cvan
Copy link
Contributor

cvan commented Sep 15, 2014

What's with the line-height in the compatibility message? How long has it been like that?

@cvan
Copy link
Contributor

cvan commented Sep 15, 2014

The FxA line-height could use some breathing:

@cvan
Copy link
Contributor

cvan commented Sep 15, 2014

at tablet width, it looks like there's too much top padding compared to bottom padding:

@mstriemer
Copy link
Contributor Author

I messed up the line height for the incompatibility banner. It had the table cell hack for vertical align before. I might have to add it back in.

@cvan
Copy link
Contributor

cvan commented Sep 15, 2014

Firefox Accounts has arrived! Sign in to transfer your account. It's that simple!

What is Firefox Accounts? Learn more

I know you didn't write that, but this is way too wordy IMO. Too much text to scan.

And you should have a period after "Learn more" too.

I'd suggest changing to this:

Firefox Accounts has arrived! Simply sign in to transfer your account.

At least drop the "It's that simple" text:

Firefox Accounts has arrived! Simply sign in to transfer your account.

What is Firefox Accounts? Learn more.

@@ -19,7 +19,6 @@
</div>

<header id="site-header" class="header site-header"></header>
<aside id="incompatibility-banner"></aside>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to remove from Zamboni

@mstriemer
Copy link
Contributor Author

Updated line height.

screen shot 2014-09-02 at 16 02 25

@mstriemer
Copy link
Contributor Author

I took a screenshot of the tablet width and zoomed in on it. Looks like it's about evenly spaced. 13px from the bottom of the link underline to the bottom and 13px from the top of the is to the top.

@cvan
Copy link
Contributor

cvan commented Sep 15, 2014

I took a screenshot of the tablet width and zoomed in on it. Looks like it's about evenly spaced. 13px from the bottom of the link underline to the bottom and 13px from the top of the is to the top.

It's 4px taller on the bottom:

@cvan
Copy link
Contributor

cvan commented Sep 15, 2014

if you are having problems centreing the text vertically, use flexbox:

http://philipwalton.github.io/solved-by-flexbox/

  /* Old flexbox to centre horizontally. */
  display: -webkit-box;
  display: -moz-box;
  display: -ms-flexbox;
  display: -webkit-flex;
  align-items: center;

  /* New flexbox to centre horizontally. */
  display: flex;
  justify-content: center;

Or look at this jsfiddle.

@cvan
Copy link
Contributor

cvan commented Sep 15, 2014

how's it look on an app detail page (e.g., /app/vtranslator)?

@mstriemer
Copy link
Contributor Author

screen shot 2014-09-02 at 16 53 58
1280x600 detail

screen shot 2014-09-02 at 16 54 04
320x480 detail

@mstriemer
Copy link
Contributor Author

I'm seeing a 1px difference on tablet width, could have been fixed by flex box. I'd imagine the 1px is room for characters that go below the line like the y and g in the sign in link.

screen shot 2014-09-02 at 17 04 34

@mstriemer
Copy link
Contributor Author

@andym Do you know who came up with the text for this banner? Can we get it revised based on cvan and chuck's comments?

@mstriemer mstriemer closed this in 9ea27d4 Sep 16, 2014
@mstriemer mstriemer deleted the fx-accounts-banner-component-1052864 branch September 16, 2014 15:42
@mstriemer
Copy link
Contributor Author

Pushed to master to keep history here. I'm open to revising the API just wanted to get this in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants