Fix Bug 1405076, firefox landing page layout and content updates #5195

schalkneethling · 2017-10-11T14:53:42Z

Description

Updates the /firefox page to mirror the current /firefox/quantum with additional copy updates.

Issue / Bugzilla link

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

Testing

Ensure that is switch is not enabled the current content hub version of the page is shown.

alexgibson

A few nits aside but this is looking great 👍

My biggest question on the page is that we're missing the Firefox sub-navigation?

I'm guessing this might be intentional from the design, but it seems like we're cutting off a vital piece of navigation to other pages/products. I would flag it as a questionable choice?

Final thing is, this change is bound to break some integration tests. I would push this branch to run-integration-tests to see what fails, and then remove the tests for the current /firefox page. It's a shame to lose coverage, but until bug 1402424 has a solution, we can't add tests for the new page until the switch is enabled.

alexgibson · 2017-10-12T08:44:30Z

bedrock/firefox/templates/firefox/firefox-quantum.html

+        <h1>{{ _('<strong>Firefox</strong> Quantum') }}</h1>
+        <h2>{{ _('New. Fast. For good.') }}</h2>
+        <div class="cta-wrapper">
+          {{ download_firefox(alt_copy=_('Download Now'), dom_id='firefox-desktop-page', download_location='firefox-header') }}


I think this should be download_location='primary cta'

alexgibson · 2017-10-12T08:45:24Z

bedrock/firefox/templates/firefox/firefox-quantum.html

+    <div class="content">
+      <div class="secondary-download-cta-content">
+        <h2>{{ _('Start browsing with <strong>Firefox</strong> Quantum') }}</h2>
+        {{ download_firefox(alt_copy=_('Download Now'), dom_id='firefox-desktop-page', download_location='firefox-secondary') }}


This should be download_location='other'

alexgibson · 2017-10-12T08:45:56Z

bedrock/firefox/views.py

-    blog_slugs = 'firefox'
-    blog_tags = ['home']
-    template_name = 'firefox/hub/home.html'
+    if switch('firefox-57-release'):


It would be nice to have some unit tests that ensure the correct template is loaded for both states of the switch

alexgibson · 2017-10-12T08:49:50Z

media/css/firefox/quantum-landing.scss

+    font-weight: normal;
+}
+
+a.download-link.button.button-green {


Rather than overriding styles, it would be nice here to make a new button style and @import it into the stylesheet. Similar to what we've done here. Give it a button name like quantum. This will make it easier for us to consolidate styles between pages in the future.

So, the reason I did the override is because I am using the download_firefox helper. Now, although you cannot specify a custom class, I see there is a parameter called button_color with a default value of button-green so, I guess I can use that as a way to specify a custom class, say

{{ download_firefox(alt_copy=_('Download Now'), dom_id='firefox-desktop-page', download_location='other', button_color='quantum-download') }}

schalkneethling · 2017-10-12T10:40:33Z

My biggest question on the page is that we're missing the Firefox sub-navigation?

@alexgibson That is my bad, I completely forgot that you have to explicitly import it to the page. Will fix that.

alexgibson · 2017-10-12T14:44:41Z

bedrock/firefox/tests/test_views.py

@@ -754,6 +754,22 @@ def test_get_context_data_one_star(self):
        self.assertFalse('donate_stars_url' in ctx)


+class TestFirefoxHubView(TestCase):


This passes for me locally:

@override_settings(DEV=False) @patch('bedrock.firefox.views.l10n_utils.render') class TestFirefoxFeatures(TestCase): @patch.object(views, 'lang_file_is_active', lambda *x: True) @patch('bedrock.firefox.views.switch', Mock(return_value=True)) def test_new_firefox_page_loads_with_active_switch(self, render_mock): view = views.FirefoxHubView() view.request = RequestFactory().get('/firefox/') view.request.locale = 'en-US' eq_(view.get_template_names(), ['firefox/firefox-quantum.html']) @patch.object(views, 'lang_file_is_active', lambda *x: False) @patch('bedrock.firefox.views.switch', Mock(return_value=False)) def test_firefox_hub_page_loads_with_inactive_switch(self, render_mock): view = views.FirefoxHubView() view.request = RequestFactory().get('/firefox/') view.request.locale = 'fr' eq_(view.get_template_names(), ['firefox/hub/home.html'])

Hmm, actually - you could probably split this test out further so the switch is tested independently of the locale activation

Oh so you mean test that if one of the two is not True then the current page should show. Will do.

alexgibson · 2017-10-12T14:45:33Z

bedrock/firefox/templates/firefox/firefox-quantum.html

+    <div class="content">
+      <div class="secondary-download-cta-content">
+        <h2>{{ _('Start browsing with <strong>Firefox</strong> Quantum') }}</h2>
+        {{ download_firefox(alt_copy=_('Download Now'), dom_id='firefox-desktop-page', download_location='other', button_color='quantum-download') }}


This is a duplicate ID: dom_id='firefox-desktop-page' - it should be different to the one in the primary cta section at the top of the page

alexgibson · 2017-10-12T14:50:34Z

Download now is not aligned vertically

alexgibson · 2017-10-12T14:51:21Z

Is this heading a little large maybe? Alternatively, could you shrink the width so "Firefox Quantum" is on the second line together?

schalkneethling · 2017-10-12T14:58:52Z

Is this heading a little large maybe? Alternatively, could you shrink the width so "Firefox Quantum" is on the second line together?

Agree

schalkneethling · 2017-10-12T15:34:14Z

Download now is not aligned vertically

I made the "Firefox Privacy Notice" a smaller font size to match the page you pointed out the other day that also uses the same button style.

alexgibson · 2017-10-12T15:43:46Z

I made the "Firefox Privacy Notice" a smaller font size to match the page you pointed out the other day that also uses the same button style.

I don't see the same issue here: https://www-demo3.allizom.org/en-US/firefox/new/

schalkneethling · 2017-10-12T15:57:25Z

I don't see the same issue here: www-demo3.allizom.org/en-US/firefox/new

@alexgibson I believe the last update solved the problem. Apologies, my mind is all over the place today...

peiying2 · 2017-10-12T18:22:13Z

@schalkneethling could you post test URL here please?

alexgibson · 2017-10-13T10:26:57Z

tests/functional/newsletter/test_newsletter_embed.py

@@ -71,6 +77,9 @@ def test_newsletter_successful_sign_up(page_class, base_url, selenium):
    assert page.newsletter.sign_up_successful


+# /firefox Quantum page behind switch, unskip when the switch is removed
+@pytest.mark.skipif(


Same here:

@pytest.mark.parametrize('page_class', [ HomePage, ContributePage, pytest.mark.skipif(HomePage, reason='https://bugzilla.mozilla.org/show_bug.cgi?id=1293779')])

Guessing the above should be:

pytest.mark.skipif(FirefoxHomePage, reason='https://bugzilla.mozilla.org/show_bug.cgi?id=1405076')])

Ah yes, my bad - thanks!

alexgibson · 2017-10-13T10:27:27Z

tests/functional/newsletter/test_newsletter_embed.py

@@ -58,6 +61,9 @@ def test_newsletter_default_values(page_class, url_kwargs, base_url, selenium):
    assert page.newsletter.is_privacy_policy_link_displayed


+# /firefox Quantum page behind switch, unskip when the switch is removed
+@pytest.mark.skipif(


Instead of skipping all the tests, skip just the failing one:

@pytest.mark.parametrize('page_class', [ HomePage, ContributePage, pytest.mark.skipif(HomePage, reason='https://bugzilla.mozilla.org/show_bug.cgi?id=1293779')])

alexgibson · 2017-10-13T10:28:11Z

tests/functional/newsletter/test_newsletter_embed.py

@@ -24,6 +24,9 @@
 from pages.smarton.base import SmartOnBasePage


+# /firefox Quantum page behind switch, unskip when the switch is removed
+@pytest.mark.skipif(
+    reason='https://bugzilla.mozilla.org/show_bug.cgi?id=1405076')


Use this to skip only the failing test:

pytest.mark.skipif((FirefoxHomePage, None), reason='https://bugzilla.mozilla.org/show_bug.cgi?id=1405076')

alexgibson · 2017-10-13T10:28:48Z

tests/functional/firefox/test_home.py

@@ -7,6 +7,9 @@
 from pages.firefox.home import FirefoxHomePage


+# /firefox Quantum page behind switch, unskip when the switch is removed


I don't think we really need the comments here, as we have the reason referencing the bug.

alexgibson

A couple of minor nits for consistency, but otherwise this looks great! r+wc 🍰

alexgibson · 2017-10-13T12:55:25Z

media/css/firefox/components/_buttons.scss

+
+        .download-title {
+            color: #0040ab;
+            @include font-size(14px);


Nit: try and and avoid using arbitraty font sizes. Suggest @include font-size-small; here?

alexgibson · 2017-10-13T12:57:52Z

media/css/firefox/quantum-landing.scss

+}
+
+.fx-privacy-link a {
+    @include font-size(14px);


Suggest @include font-size-extra-small; here? Could this also be moved to _buttons.scss?

alexgibson · 2017-10-13T13:03:28Z

media/css/firefox/components/_buttons.scss

+        .download-title {
+            color: #0040ab;
+            @include font-size(14px);
+            font-weight: 700;


Any reason not just to use font-weight: bold;?

Nope, I was using the same code as from the reference page that uses the same button style but, bold would work just as well.

schalkneethling · 2017-10-13T14:08:50Z

Thanks for the detailed review @alexgibson - updated.

alexgibson · 2017-10-13T14:11:54Z

bedrock/firefox/views.py

+        locale = l10n_utils.get_locale(self.request)
+
+        if switch('firefox-57-release') and lang_file_is_active(
+                'firefox/firefox-quantum.lang', locale):


Appologies I missed this before, it should be firefox/firefox-quantum? (no .lang)

schalkneethling requested a review from alexgibson October 11, 2017 14:53

schalkneethling added the L10N label Oct 11, 2017

alexgibson requested changes Oct 12, 2017

View reviewed changes

flodolo added L10N - Strings Extracted and removed L10N labels Oct 13, 2017

alexgibson requested changes Oct 13, 2017

View reviewed changes

alexgibson reviewed Oct 13, 2017

View reviewed changes

Fix Bug 1405076, firefox landing page layout and content updates

55a60cb

alexgibson approved these changes Oct 13, 2017

View reviewed changes

alexgibson merged commit 27b5b1c into mozilla:master Oct 13, 2017

alexgibson mentioned this pull request Oct 16, 2017

[bug 1405076] Revert Firefox Quantum home page design #5209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bug 1405076, firefox landing page layout and content updates #5195

Fix Bug 1405076, firefox landing page layout and content updates #5195

schalkneethling commented Oct 11, 2017

alexgibson left a comment

alexgibson Oct 12, 2017

alexgibson Oct 12, 2017

alexgibson Oct 12, 2017

alexgibson Oct 12, 2017

schalkneethling Oct 12, 2017

schalkneethling commented Oct 12, 2017 •

edited

alexgibson Oct 12, 2017

alexgibson Oct 12, 2017

schalkneethling Oct 12, 2017

alexgibson Oct 12, 2017

schalkneethling Oct 12, 2017

alexgibson commented Oct 12, 2017

alexgibson commented Oct 12, 2017

schalkneethling commented Oct 12, 2017

schalkneethling commented Oct 12, 2017

alexgibson commented Oct 12, 2017

schalkneethling commented Oct 12, 2017

peiying2 commented Oct 12, 2017

alexgibson Oct 13, 2017

schalkneethling Oct 13, 2017

alexgibson Oct 13, 2017

alexgibson Oct 13, 2017

alexgibson Oct 13, 2017

alexgibson Oct 13, 2017

alexgibson left a comment

alexgibson Oct 13, 2017

alexgibson Oct 13, 2017

alexgibson Oct 13, 2017

schalkneethling Oct 13, 2017

schalkneethling commented Oct 13, 2017

alexgibson Oct 13, 2017

schalkneethling Oct 13, 2017

		@@ -754,6 +754,22 @@ def test_get_context_data_one_star(self):
		self.assertFalse('donate_stars_url' in ctx)


		class TestFirefoxHubView(TestCase):

		@@ -7,6 +7,9 @@
		from pages.firefox.home import FirefoxHomePage


		# /firefox Quantum page behind switch, unskip when the switch is removed

Fix Bug 1405076, firefox landing page layout and content updates #5195

Fix Bug 1405076, firefox landing page layout and content updates #5195

Conversation

schalkneethling commented Oct 11, 2017

Description

Issue / Bugzilla link

Testing

alexgibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkneethling commented Oct 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson commented Oct 12, 2017

alexgibson commented Oct 12, 2017

schalkneethling commented Oct 12, 2017

schalkneethling commented Oct 12, 2017

alexgibson commented Oct 12, 2017

schalkneethling commented Oct 12, 2017

peiying2 commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexgibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkneethling commented Oct 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkneethling commented Oct 12, 2017 •

edited