[fix bug 1325198] Test /all links on /new, part 2. #4550

jpetto · 2016-12-28T16:35:36Z

Description

2nd round of testing for alternate download links on /firefox/new/. en-US only.

Version 1: - Link added underneath Firefox Privacy link that opens up a modal (winner from last test).

Version 2: - Majority of content from modal in v1, displayed directly on the page (underneath primary CTA).

I believe we want to run this test ASAP, but should confirm with other teams that no other upcoming experiments (i.e. Optimizely/funnelcake) would conflict.

Bugzilla link

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

Testing

Ensure UI works across browsers (SVGs wont be displayed on old IE). Make sure direct download links work, and that experiment works as intended.

Checklist

Related functional & integration tests passing.

craigcook · 2016-12-30T22:44:21Z

Some layout issues when the refresh button is shown and the download button container floats left. I'm not sure what circumstances show the refresh button. I got it locally but don't see it on demo, but maybe this scenario isn't possible in production?

craigcook · 2016-12-30T22:51:19Z

I definitely get the refresh button in production in my current version. If you have trouble triggering it locally you can just manually add the show-refresh class to the html element.

craigcook

A few layout issues but looking good to me otherwise. Maybe worth getting a second opinion from @alexgibson on the JavaScript bits.

craigcook · 2016-12-30T22:59:32Z

bedrock/firefox/templates/firefox/new/scene1.html

@@ -61,6 +69,12 @@
              <div class="mobile download-button-wrapper" id="download-button-wrapper-mobile" data-upgrade-subtitle="{{_('Get it free on Google Play')}}">
                {{ download_firefox(dom_id='download-button-mobile') }}
              </div>
+
+            {% if version == '2' %}
+              <div id="test-inpage-wrapper">


I think this probably needs to move down after #refresh-firefox-wrapper so it can clear the floats when both buttons are shown.

Ah, yeah, tested in Beta and Dev Edition. 😞 Moving this block to the last element in main-content seems to do the trick.

craigcook · 2016-12-30T23:10:47Z

media/css/firefox/new/all-test-variations-pt2.less

+    li {
+        .border-box();
+        margin-bottom: 10px;
+        width: 50%;


I think these may also need to float left instead of being inline-block. In smaller viewports "Windows (XP/Vista)" wraps to two lines and "OSX" is misaligned. I think floating the list items lets the text align top but there may be another fix too.

Based on conversation below, the XP/Vista link will be merged with the Windows 32-bit link, which will skirt us around this issue for now.

alexgibson · 2017-01-03T09:16:59Z

I definitely get the refresh button in production in my current version. If you have trouble triggering it locally you can just manually add the show-refresh class to the html element.

You need to enable UITour in order to see the refresh button, and then view the page using the current release version of Firefox (may also need to update product details locally)

alexgibson · 2017-01-03T09:32:05Z

media/js/firefox/new/all-test-variations-pt2.js

+        case 'Linux 64-bit':
+            $li.attr('data-sort', 4);
+            break;
+        case 'OS X':


Not related to this change, but I guess we should really file a bug to rename OS X to macOS sometime soon?

Probably a good idea. Hopefully Mac users don't get confused. 😒

alexgibson · 2017-01-03T09:34:12Z

media/js/firefox/new/all-test-variations-pt2.js

+        var $html = $('html');
+
+        // make sure desktop download button exists, user is not on Android or iOS, and user is on a recognized platform
+        if (!$html.hasClass('ios') && !$html.hasClass('android') && $dlButton.length && $dlButton.find('.unrecognized-download:visible').length === 0) {


Is there any reason why we don't want to show this for iOS and Android users? At the very least, iOS users would also see there is an Android version available, and vice-versa?

Could this maybe be simplified to say "as long as there is a download button visible, show the extra content"?

I think the reason we went desktop only here was to make sure the links are actionable. Clicking on the Google Play button while on iOS would probably be more frustrating than helpful.

I agree we may want to promote/display information about Android on iOS (and vice versa), but I think that's a different task than we're performing here.

(FWIW, my preference/hope is for a single link to a freshened up /firefox/all/ instead of the handful of links we're testing here.)

alexgibson · 2017-01-03T09:35:58Z

media/js/firefox/new/all-test-variations-pt2.js

+        // make sure desktop download button exists, user is not on Android or iOS, and user is on a recognized platform
+        if (!$html.hasClass('ios') && !$html.hasClass('android') && $dlButton.length && $dlButton.find('.unrecognized-download:visible').length === 0) {
+            // pull the nojs links out of the primary CTA
+            var $directLis = $dlButton.find('.nojs-download li').remove();


I totally get that this is for a test and is quick and dirty, but a lot of this DOM-fu could probably be avoided if we wrote a macro specifically for this use-case?

Agreed, but I'm holding off on that until we have a final direction. I really hope we go with a single link to an improved /firefox/all/ rather than injecting direct links here.

alexgibson · 2017-01-03T09:57:49Z

media/css/firefox/new/all-test-variations-pt2.less

+    }
+}
+
+#test-mobile-downloads {


It looks like these mobile links need to be block-level and have a set height/width to show up on IE < 9

👍 Will fix.

alexgibson · 2017-01-03T10:02:29Z

bedrock/settings/static_media.py

@@ -1278,6 +1285,20 @@
        ),
        'output_filename': 'js/firefox_new_scene2-bundle.js',
    },
+    'experiment_firefox_new_scene1_all_links-pt2': {


This should be an underscore before pt2? e.g. experiment_firefox_new_scene1_all_links_pt2

alexgibson · 2017-01-03T10:07:47Z

media/js/firefox/new/all-test-variations-pt2.js

+                $dlButton.append($newLink);
+            // move forest up on the page a bit (original value = -254px)
+            } else if (version === 2) {
+                $('.forest-container').css('margin-top', '-310px');


Moving this content up is fine as long as the download buttons are shown, but if you set the HTML classes linux arm for example, then the install link can no longer be clicked because the horizon assets cover the content.

Good call. There are too many display variations to be this cute about spacing. Will remove the special casing here.

alexgibson · 2017-01-03T10:12:56Z

media/js/firefox/new/all-test-variations-pt2.js

+            $li.attr('data-sort', 5);
+            break;
+        default:
+            $li.attr('data-sort', 99);


Currently Windows (XP/Vista) is falling through to this ordering. Do we need to special case it? Ideally, Windows users should only see the link that is appropriate to them providing we have JS.

Before the special XP/Vista link was added, Verdi requested the specific ordering (I believe) based on popularity. That being the case, I think XP/Vista being last is probably correct.

I think keeping the XP/Vista link visible for all users stays true to the original intent of the test, but am open to discussion if there's a potential for harm having all 3 Windows downloads displayed. Are you thinking we should do some detection and show either both Windows 32 and Windows 64 OR only XP/Vista?

This is a bit of a special edge case, as it's not really like a specific platform link like the others - it just points to a specific bouncer end-point that supports sha-1. For Windows XP users they still want "Windows 32 bit" for example, but the main link won't work at all as it points to the sha-256 bouncer URL. When they click "Windows XP/Vista", they still get the 32 bit Windows installer. It's a little confusing with 2 buttons pointing to the same product imho, but will defer to what you think is most appropriate.

If it might be of any help, I seem to remember adding special casing for the original experiment in the sha-1 PR: #4413

Ah, I see the difference now. Thanks for the link to the previous PR - will pilfer.

jpetto · 2017-01-03T21:57:29Z

Pushed an update that I believe addresses all the comments. @craigcook + @alexgibson - one more look?

alexgibson · 2017-01-04T09:36:27Z

Still seems to be an alignment issue for experiment v=1 when both download and refresh buttons are shown:

alexgibson · 2017-01-04T09:38:15Z

Other than the one layout issue, the changes all look good to me. r+wc 👍

alexgibson · 2017-01-04T09:43:58Z

I don't know if it's even worth spending much time on this for an experiment, but I noticed in IE7 (cringe), the download button in the modal is not displayed:

Haven't dug in, but it might be something that is magically fixed by using the inline-block; mixin.

jpetto · 2017-01-04T16:08:39Z

Fixed the ?v=1 link UI and the IE 7 display bug (float container just needed position: relative). Hopefully we're good now. 😣

alexgibson

r+ to merge once squashed 👍

jpetto added the Durable Team Sprint label Dec 28, 2016

craigcook self-assigned this Dec 28, 2016

jpetto force-pushed the bug-1325198-fxnew-all-download-test-pt2 branch 2 times, most recently from 9576a60 to 1409dfe Compare December 29, 2016 19:30

craigcook requested changes Dec 30, 2016

View reviewed changes

alexgibson self-requested a review January 3, 2017 09:17

alexgibson reviewed Jan 3, 2017

View reviewed changes

jpetto force-pushed the bug-1325198-fxnew-all-download-test-pt2 branch from cbe9d27 to a4bcdc9 Compare January 3, 2017 22:00

alexgibson approved these changes Jan 4, 2017

View reviewed changes

[fix bug 1325198] Test /all links on /new, part 2.

01296a1

jpetto force-pushed the bug-1325198-fxnew-all-download-test-pt2 branch from 4ab6c3d to 01296a1 Compare January 4, 2017 16:37

craigcook approved these changes Jan 4, 2017

View reviewed changes

alexgibson merged commit 44b75cc into mozilla:master Jan 4, 2017

jpetto deleted the bug-1325198-fxnew-all-download-test-pt2 branch April 18, 2018 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix bug 1325198] Test /all links on /new, part 2. #4550

[fix bug 1325198] Test /all links on /new, part 2. #4550

jpetto commented Dec 28, 2016

craigcook commented Dec 30, 2016

craigcook commented Dec 30, 2016

craigcook left a comment

craigcook Dec 30, 2016

jpetto Jan 3, 2017

craigcook Dec 30, 2016

jpetto Jan 3, 2017

alexgibson commented Jan 3, 2017

alexgibson Jan 3, 2017

jpetto Jan 3, 2017

alexgibson Jan 3, 2017

alexgibson Jan 3, 2017

jpetto Jan 3, 2017

alexgibson Jan 3, 2017

jpetto Jan 3, 2017

alexgibson Jan 3, 2017

jpetto Jan 3, 2017

alexgibson Jan 3, 2017

jpetto Jan 3, 2017

alexgibson Jan 3, 2017

jpetto Jan 3, 2017

alexgibson Jan 3, 2017

jpetto Jan 3, 2017

alexgibson Jan 3, 2017

alexgibson Jan 3, 2017 •

edited

jpetto Jan 3, 2017

jpetto commented Jan 3, 2017

alexgibson commented Jan 4, 2017

alexgibson commented Jan 4, 2017

alexgibson commented Jan 4, 2017

jpetto commented Jan 4, 2017

alexgibson left a comment

[fix bug 1325198] Test /all links on /new, part 2. #4550

[fix bug 1325198] Test /all links on /new, part 2. #4550

Conversation

jpetto commented Dec 28, 2016

Description

Bugzilla link

Testing

Checklist

craigcook commented Dec 30, 2016

craigcook commented Dec 30, 2016

craigcook 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

alexgibson commented Jan 3, 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

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

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 Jan 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpetto commented Jan 3, 2017

alexgibson commented Jan 4, 2017

alexgibson commented Jan 4, 2017

alexgibson commented Jan 4, 2017

jpetto commented Jan 4, 2017

alexgibson left a comment

Choose a reason for hiding this comment

alexgibson Jan 3, 2017 •

edited