open iframe on first run, sign in, animate#4579
Conversation
|
Thanks! I'll take a look. Looks like some stuff I'd like @alexgibson to check out as well. |
alexgibson
left a comment
alexgibson
left a comment
There was a problem hiding this comment.
A good start 👍 I left some comments that may hopefully be useful in terms of direction. Will take another look when you're ready.
| <div> | ||
| <img src="/media/img/firefox/firstrun-horizon/sample.png"> | ||
| </div> | ||
| <a href = "https://www.mozilla.org/en-US/firefox/sync/"> Learn more about Firefox Accounts</a> |
There was a problem hiding this comment.
Use the url helper for internal links e.g. href="{{ url('firefox.sync') }}". Also remove the space either side of the =.
| <div id="left-divider"> | ||
| <p>Sign in to your account and we'll sync the bookmarks, passwords and other great things you've saved to Firefox on other devices.</p> | ||
| <div> | ||
| <img src="/media/img/firefox/firstrun-horizon/sample.png"> |
There was a problem hiding this comment.
We should use the static media helper for all referenced images in templates. This ensures we can use the correctly cached bundle names in production. e.g. src="{{ static('img/firefox/firstrun-horizon/sample.png') }}"
| <div> | ||
| <div id="scene"> | ||
| <div class="fxaccounts-container"> | ||
| <h2 id="title"> Already Using Firefox?</h2> |
There was a problem hiding this comment.
Please wrap all text for l10n. I understand this is a test for en-US only, but this is general practise and also saves work further down the road when we decide to start translating the page. e.g. {{_('Already Using Firefox?')}}. Note you also have a stray space here before Already.
There was a problem hiding this comment.
Should this probably be an <h1> too?
| </div> | ||
| </div> | ||
|
|
||
| <img id="night-sky" src="/media/img/firefox/firstrun-horizon/night-scene/sky.png"> |
There was a problem hiding this comment.
Not a blocker seeing as this is just a test, but because these images are all presentation, it might be better they we're a bunch of <div>'s with background images in CSS. At least for now, please include an empty alt="" attribute for each image so that screen readers will avoid reading out the filenames.
| {% endblock %} | ||
|
|
||
| {% block site_js %} | ||
| {% javascript 'site' %} |
There was a problem hiding this comment.
The site bundle should be in the <head> of the page, so things like Google Analytics will work correctly. You would get this kind of setup for the page for free if you extended the firefox/base-resp.html template instead of bare.html. The page is also missing a lot of other meta type info because of this. Switching would require some more work though to get things looking correct again, so it's up to you here...
| let skipbutton = document.getElementById('skip-button'); | ||
|
|
||
| let onVerificationComplete = () => { | ||
| scene.setAttribute('sign-in', 'true'); |
There was a problem hiding this comment.
This would be a good use-case for data- attributes?
| if (event.propertyName === 'transform') { | ||
| window.setTimeout(function () { | ||
| window.location.href ="https://github.com/"; | ||
| // Mozilla.UITour.showNewTab(); // TODO someday |
There was a problem hiding this comment.
This isn't working because you're not importing uitour-lib.js
There was a problem hiding this comment.
side note - it works now - the issue was permission to access uitour from 127.0.0.1:8111
| }); | ||
| }); | ||
|
|
||
| scene.setAttribute('sunrise', 'true'); |
There was a problem hiding this comment.
Again, use data- attributes here and below.
| </div> | ||
|
|
||
| <img id="night-sky" src="/media/img/firefox/firstrun-horizon/night-scene/sky.png"> | ||
| <img id="sky" src="/media/img/firefox/firstrun-horizon/day-scene/sky.png"> |
There was a problem hiding this comment.
Is there a reason why these image are no longer SVG too? I see a bunch of @2x pngs in the PR, but they are not being used currently. Might SVG yield a faster page load and better performance?
There was a problem hiding this comment.
yes, I will remove all of the x2 pngs
I suppose it's arguable, but I was working under the impression that SVGs are less efficient, especially when animating.
There was a problem hiding this comment.
My main concern there is the total page weight using png's. I'll see if we can get this up on a demo server soon and take a look performance-wise.
| </div> | ||
| <div class="fxaccounts" id="fxa-iframe-config" data-host="{{ settings.FXA_IFRAME_SRC }}" data-mozillaonline-host="{{ settings.FXA_IFRAME_SRC_MOZILLAONLINE }}"> | ||
| <iframe id="fxa" scrolling="no" data-src="{{ settings.FXA_IFRAME_SRC }}signin?utm_campaign=fxa-embedded-form&utm_medium=referral&utm_source=firstrun&utm_content=fx-{{ version }}&entrypoint=firstrun&service=sync&context=iframe&style=chromeless&haltAfterSignIn=true"></iframe> | ||
| <button id="skip-button"> Skip This Step</button> |
There was a problem hiding this comment.
Extra space here before Skip
|
It would be nice if this page was responsive too. It's shown on desktop only, but we found the existing |
| <img id="mountains-trees" src="/media/img/firefox/firstrun-horizon/day-scene/mountains-trees.png"> | ||
| <img id="night-foreground-fox" src="/media/img/firefox/firstrun-horizon/night-scene/foreground-fox.png"> | ||
| <img id="foreground-fox" src="/media/img/firefox/firstrun-horizon/day-scene/foreground-fox.png"> | ||
| <img id="scene-blur" src="/media/img/firefox/firstrun-horizon/scene-blur.png"> |
There was a problem hiding this comment.
This image is pretty huge - would achieving the same effect be possible with CSS filters perhaps? May well be out of scope in the timeframe here, but just a thought.
There was a problem hiding this comment.
CSS filter (blur) slowed down the performance drastically - so this was done to get around that.
| document.getElementById('sunrise').addEventListener('transitionend', function(event) { | ||
| if (event.propertyName === 'transform') { | ||
| window.setTimeout(function () { | ||
| window.location.href ="https://github.com/"; |
There was a problem hiding this comment.
was temporarily using github as a placeholder, now about:newtab works so has been changed to Mozilla.UITour.showNewTab();
|
@alexgibson Great Comments, very thorough, thank you so much! |
bedrock/settings/base.py
Outdated
| "'unsafe-inline'", | ||
| ) | ||
| CSP_CHILD_SRC = ( | ||
| 'stomlinson.dev.lcip.org', |
It is....well, the modal is not, but the rest is. I'll work on the modal. |
| @@ -0,0 +1,37 @@ | |||
| {% extends "bare.html" %} | |||
| {% stylesheet 'firefox_new_firstrun' %} | |||
There was a problem hiding this comment.
This should be in a block called {% block site_css %}{% endblock %}. Currently this is adding the stylesheets before <!doctype html>, which is invalid.
| {% extends "bare.html" %} | ||
| {% stylesheet 'firefox_new_firstrun' %} | ||
| {% block site_js %} | ||
| {% javascript 'site' %} |
There was a problem hiding this comment.
This site bundle is currently being loaded twice, as it's already being included in the base-resp.html template which bare.html extends. You can remove this line.
| }, | ||
| 'firefox_new_firstrun': { | ||
| 'source_filenames': ( | ||
| 'js/firefox/firstrun/new-firstrun.js', |
There was a problem hiding this comment.
new-firstrun.js should probably come last in the bundle?
|
|
||
| (function (Mozilla) { | ||
| 'use strict'; | ||
| let scene = document.getElementById('scene'); |
There was a problem hiding this comment.
CircleCI is still failing when running collectstatic (which concatenates and minifies our JS). I'm guessing it's because there is still some ES6 variable names here and below.
There was a problem hiding this comment.
Oh is that why its failing! I was trying to figure that out - thanks
| <div class="fxaccounts-container"> | ||
| <h1 id="title">{{_('Already Using Firefox?')}}</h1> | ||
| <div id="left-divider"> | ||
| <p>{{_('Sign in to your account and we\'ll sync the bookmarks, passwords and other great things you\'ve saved to Firefox on other devices.')}}</p> |
There was a problem hiding this comment.
Nit: for consistency with other page copy on mozorg you can use curly quotes ’ here instead of escaping.
| <div id="left-divider"> | ||
| <p>{{_('Sign in to your account and we\'ll sync the bookmarks, passwords and other great things you\'ve saved to Firefox on other devices.')}}</p> | ||
| <div> | ||
| <img src="{{ static('img/firefox/firstrun-horizon/sample.png') }}"> |
There was a problem hiding this comment.
Please add an empty alt=""
| </div> | ||
| </div> | ||
|
|
||
| <img id="night-sky" alt=" Night Sky" src="{{ static('img/firefox/firstrun-horizon/night-scene/sky.png') }}"> |
There was a problem hiding this comment.
I don't think adding alt text for these images makes a whole lot of sense, since they are more just for presentation, and having a screen reader read through these as a list won't really add a lot of meaning to the page imho. Please use empty alt="" attributes.
bedrock/firefox/views.py
Outdated
| if detect_channel(version) == 'alpha': | ||
| template = 'firefox/dev-firstrun.html' | ||
| elif show_40_firstrun(version): | ||
| #New onboarding animation |
bedrock/settings/static_media.py
Outdated
| 'css/firefox/firstrun/new-firstrun.less', | ||
| 'css/base/mozilla-fxa-iframe.less', | ||
| ), | ||
| 'output_filename': 'css/firefox_new-firstrun-bundle.css', |
| overflow: hidden; | ||
| margin: auto; | ||
| position: absolute; | ||
| top: 0; left: 0; bottom: 0; right: 0; |
There was a problem hiding this comment.
Nit: please use a separate line per property
bedrock/firefox/views.py
Outdated
| if detect_channel(version) == 'alpha': | ||
| template = 'firefox/dev-firstrun.html' | ||
| elif show_40_firstrun(version): | ||
| #New onboarding animation funnecake |
There was a problem hiding this comment.
Looks like this is causing the flake 8 error: bedrock/firefox/views.py:385:13: E265 block comment should start with '# '
|
Hi @ericawright - we just landed #4567 which adds CSS linting to our CI. Would you mind rebasing this branch against up to date master so we can use that here in this PR? There are some instructions on how to rebase here if you need: http://bedrock.readthedocs.io/en/latest/contribute.html#git-workflow Thanks! |
|
@alexgibson has been rebased |
| </div> | ||
| <div id="firefox-logo"></div> | ||
| <div class="fxaccounts" id="fxa-iframe-config" data-host="{{ settings.FXA_IFRAME_SRC }}" data-mozillaonline-host="{{ settings.FXA_IFRAME_SRC_MOZILLAONLINE }}"> | ||
| <iframe id="fxa" scrolling="no" data-src="{{ settings.FXA_IFRAME_SRC }}signin?utm_campaign=fxa-embedded-form&utm_medium=referral&utm_source=firstrun&utm_content=fx-{{ version }}&entrypoint=firstrun&service=sync&context=iframe&style=chromeless&haltAfterSignIn=true"></iframe> |
There was a problem hiding this comment.
Hey, for FxA metrics could we please specify a different entrypoint= value here?
|
Hi @ericawright - I pushed your branch to a demo for testing: https://www-demo1.allizom.org/en-US/firefox/51.0/firstrun/?f=99 I also created a mozorg bug to track this change here: |
|
@alexgibson did you want that deploy to use one of our FxA dev boxes? Right now it is using production and the form does not show up. |
|
A couple of things I noticed:
|
Ugh, I forgot the iframe doesn't work on our ad-hoc demo deploys. I'll need to switch over to one of our regular demo instances. Thanks for the reminder. |
|
@alexgibson Thank you! |
👍 🎉 |
I've also noticed that - which (along with being not pretty) causes the modal to not show up at all - will change that. |
|
OK, this is now up on demo1: https://www-demo1.allizom.org/en-US/firefox/51.0/firstrun/?f=99 |
|
@alexgibson hm our |
We've previously been able to test on demo1 through to demo5, is this a recent change? I also noticed our CSP is blocking the iframe, which is something we probably need to fix: |
|
FWIW, FxA stage is currently working on demo4, though we did have to adjust the deis config to get CSP compliant. |
Thanks @jpetto - I logged into deis and set the CSP environment variable for https://accounts.stage.mozaws.net. The iframe is now loading as expected on demo1. @vladikoff - it would be great to try and get this working on demo1-5 if possible. Is that something you can do without too much effort? |
|
From a UX perspective, if the user does not already have an account and instead chooses "sign up", should the "skip this step" button be hidden once the user starts the sign up flow? (is this possible?).
|
|
I've tested the FxA flow on demo1 using these instructions and both sign in and sign up seem to be working as expected. Other than a couple of things I noticed above, it seems to be working nicely 👍 |
Is it possible, and it's a good idea! |
@jpetto - any idea if it's possible to detect when the signup flow has begun, but not yet at the verification step? If feels like the answer is probably no, but this would be a nice UX improvement. |
|
@ericawright - at the very least if looks like you could hide the button when it gets to the verification step: https://github.com/mozilla/bedrock/blob/master/media/js/base/mozilla-fxa-iframe.js#L133 |
|
@alexgibson yeah, I'm using the signupMustVerify callback now. I don't think I can at the "choose what to sync" stage |
|
@ericawright - could you please add a couple of tests for the view logic for these funnelcakes? You can see some existing examples here: https://github.com/mozilla/bedrock/blob/master/bedrock/firefox/tests/test_base.py#L364-L388. Thanks! |
So this styling is inside of the iFrame, and I'm unable to edit it, or apply my own styling to it. I'm not certain if it's the styling from the test iframe only, or if it would be on the actual one too. I'm not entirely sure who to ask about this? @vladikoff? |
npm-debug.log
Outdated
| @@ -0,0 +1,27 @@ | |||
| 0 info it worked if it ends with ok | |||
There was a problem hiding this comment.
Think you may have committed this accidentally?
There was a problem hiding this comment.
Looks like this may have crept back again
|
Erica, unless there's anything outstanding could you please squash the commits here, and also reference 'bug 1333410' in the commit message? If you include something like (If you're not used to doing any of this, I can also probably do it for you). Thanks! |
| <iframe id="fxa" scrolling="no" data-src="{{ settings.FXA_IFRAME_SRC }}signin?utm_campaign=fxa-embedded-form&utm_medium=referral&utm_source=firstrun&utm_content=fx-{{ version }}&entrypoint=firstrunwow{{ funnelcake_id }}&service=sync&context=iframe&style=chromeless&haltAfterSignIn=true"></iframe> | ||
| <button id="skip-button">{{_('Skip this step')}}</button> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Nit: please fix indentation
| {% endblock %} | ||
|
|
||
| {% block content %} | ||
| <div> |
There was a problem hiding this comment.
Nit: this <div> doesn't seem to be used by anything?
| template = render_mock.call_args[0][1] | ||
| eq_(template, ['firefox/firstrun/firstrun-horizon.html']) | ||
|
|
||
| # New Firstrun Wow-moment funnelcake tests |
There was a problem hiding this comment.
Could we please add another test here just to make sure the control page is shown for f=98? thanks
There was a problem hiding this comment.
Oh, nevermind I see you have one below 👍
|
Other than the couple of nits above, this is an r+ from me. Fwiw, I seem to be getting a JS error when trying to create a new account on the demo server. The error originates from the FxA iframe code however, so I don't think it has anything to do with this PR directly. It was working ok when I tried on Tuesday. This is the error: https://pastebin.mozilla.org/8972345 |
I popped into #fxa on IRC and they confirmed and fixed the error on staging. Tested again and things are working as expected 😄 |
|
@alexgibson thanks for all that. There were a few changes yesterday evening. The timing has changed on the animations and the landscape was split into two images to get some more depth during zoom in. After this, there should be no more changes! |
|
New animation looks good 👍 Please remove the |
ericawright
force-pushed
the
master
branch
3 times, most recently
from
9eeed67 to
0ee7912
Compare
ericawright
force-pushed
the
master
branch
2 times, most recently
from
96bc2d6 to
645b187
Compare
|
Nice! 👍 |
5 participants
work-in-progress
@pmac
Description
Add animation to onboarding experience/ firstrun page.
Change sign-up form to sign-in instead
Animation drops user off on
about:newtabpageBased on https://dl.dropboxusercontent.com/u/105549/mvp1.framer/index.html
Bugzilla link
https://bugzilla.mozilla.org/show_bug.cgi?id=1322718
https://bugzilla.mozilla.org/show_bug.cgi?id=1333410