Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Bug 836462 - Move Thunderbird landing page to Bedrock #3217

Closed
wants to merge 2 commits into from
Closed

Fix Bug 836462 - Move Thunderbird landing page to Bedrock #3217

wants to merge 2 commits into from

Conversation

kyoshino
Copy link
Contributor

@flodolo
Copy link
Contributor

flodolo commented Aug 18, 2015

.lang files are now tracked, and pushed on stage and prod. Also added en-US reference file, and some locales that ship with Thunderbird and were not covered by the import

@schalkneethling
Copy link

One thing I have noticed so far:

screen shot 2015-08-19 at 12 34 23

  1. The wrapper should not be sized down as it is here because it throws of the alignment of Tabzilla. Instead I would suggest tweaking the width of the main container for the mobile view/layout.

Perhaps something like:

@media only screen and (max-width: 760px) {
  #wrapper {
    width: 100%;
    padding: 0;
  }
 #main-feature {
    margin: 20px auto 40px;
    padding: 0 10px;
   max-width: 600px;
  }

@schalkneethling
Copy link

That will then probably also require some tweaks to the footer.

@@ -190,10 +190,7 @@ <h1 itemprop="name" class="title-shadow-box">Bug Bounty Program FAQ</h1>
the system requirements for the

Choose a reason for hiding this comment

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

The "the" in this sentence is incorrect but, it is already out there so, not sure if we can remove it at this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the extra "the". Should be okay because the change is trivial and this page is not localized.

@kyoshino
Copy link
Contributor Author

Thanks @schalkneethling, looks like the wrapper issue is site-wide. The Firefox landing page has the same bug. We could fix it in a follow-up PR to improve the mobile landscape layout.

background: -ms-radial-gradient(center, ellipse cover, rgba(234,239,242,1) 0%, rgba(212,221,228,.5) 60%),
-ms-linear-gradient(top, rgba(202,225,244,1) 0%, rgba(125,185,232,0) 100%);
background: radial-gradient(ellipse at center, rgba(234,239,242,1) 0%,rgba(212,221,228,.5) 50%),
linear-gradient(to bottom, rgba(202,225,244,1) 0%,rgba(125,185,232,0) 100%);

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I just copied and pasted that part from firefox/landing.css. It's a little bit complex so we cannot use the mixin I guess...

Choose a reason for hiding this comment

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

Perhaps that is the case. I reckon it is ok for now. The one thing I will not is that there is currently a bug to remove the use of -moz- prefix for gradients [https://bugzilla.mozilla.org/show_bug.cgi?id=1189121] so, perhaps remove those here as part of this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove the -moz-prefixed gradients.

@jpetto
Copy link
Contributor

jpetto commented Aug 20, 2015

This, along with the other Thunderbird related PRs, is currently up on demo4: https://www-demo4.allizom.org/en-US/thunderbird/

@schalkneethling
Copy link

Tested on Fx, Chrome, Safari and IE. Other than the comment regarding removing the -moz- prefix, this looks good to me. Great stuff @kyoshino r+

@schalkneethling
Copy link

@jpetto If there is nothing else you have picked up, I reckon we are good to merge this one.

:param ctx: context from calling template.
:param channel: name of channel: 'release', 'beta' or 'alpha'.
:param small: Display the small button if True.
:param icon: Display the Fx icon on the button if True.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like this is supported at the moment, so maybe remove or comment out this param for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the icon param.

@jpetto
Copy link
Contributor

jpetto commented Aug 21, 2015

Should run the images through a compressor. I did a quick test with ImageAlpha/ImageOptim combo and saved over 50% on the screenshots.

@jpetto
Copy link
Contributor

jpetto commented Aug 21, 2015

I hate bringing up old IE issues, but IE 7 is a bit broken:

screen shot 2015-08-21 at 9 26 00 am

Not sure how much this matters, but it looks like it might be an easy fix.

@schalkneethling
Copy link

Ooooh @jpetto Good catch, I did not test IE7 to be honest, IE8+ only.

@kyoshino
Copy link
Contributor Author

Fixed the odd IE7 layout and optimized the images with ImageAlpha.

@kyoshino
Copy link
Contributor Author

Added localized images. (optimized)

@schalkneethling
Copy link

Tested in IE7, all looks good. @jpetto you happy?

@jpetto
Copy link
Contributor

jpetto commented Aug 24, 2015

Looking good to me. Pinging @pmclanahan to double check the Python bits.

@pmclanahan
Copy link
Contributor

I think the Python looks good. I'd like for there to be tests for the new download_thunderbird helper, and they can likely be mostly copied from the Firefox one. Other than that I seems about ready.

@kyoshino
Copy link
Contributor Author

Thanks @pmclanahan. As I wrote above, the helper is partially broken because we don't have the Earlybird data in product-details. For the Release channel, it should be no problem. I'll implement a workaround and add tests for that in the upcoming PR for the channel and all-beta page migration.

@schalkneethling
Copy link

@pmclanahan r?

@jpetto
Copy link
Contributor

jpetto commented Aug 24, 2015

If tests are coming in one of the other Thunderbird PRs, then I think we're good to merge?

@pmclanahan
Copy link
Contributor

That still leaves us with a completely untested download button for a product. It just makes me nervous. I'm sure it's better than the old PHP one, but can we at least add tests that verify that the things we expect it to do right now work?

@kyoshino
Copy link
Contributor Author

Added tests! Beta and Earlybird are out of the coverage for now, but this is better than nothing, as @pmclanahan said. r?

@pmclanahan
Copy link
Contributor

Thanks @kyoshino! Let's ship it :shipit: r+

@schalkneethling
Copy link

Manually merged: 33a5ff3

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

Successfully merging this pull request may close these issues.

5 participants