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 1243890] Update FxA iframe param for Fx46. #3851

Merged
merged 1 commit into from Feb 12, 2016

Conversation

@jpetto
Copy link
Contributor

@jpetto jpetto commented Feb 11, 2016

Have not yet updated firefox/accounts.js as I wanted to first get a sanity check on the approach.

Code is up on demo5:

https://www-demo5.allizom.org/en-US/firefox/46.0/firstrun/

Change isn't immediately obvious, so I'm logging the FxA iframe src to the console. If Fx 46+, src string should contain context=fx_firstrun_v2. Otherwise, context=iframe.

Actual change - after verifying email, Fx 46+ users see a blue "Sync Preferences" button in the iframe. Fx < 46 users do not get this button.

@alexgibson - Pre-review on approach? What do you think about the UITour fallback and timing?


console.log('fx version = ' + version);

if (version >= 46) {

This comment has been minimized.

@alexgibson

alexgibson Feb 12, 2016
Member

I may well be missing something, but if you're only after the major version number then using UITour for this could be overkill perhaps? If you needed the minor version or channel name then UITour is the only way, but if the major version number is the only thing of importance, then I would probably use Mozilla.Client.FirefoxMajorVersion instead. This is both synchronous and does not require any special config.

This comment has been minimized.

@jpetto

jpetto Feb 12, 2016
Author Contributor

I went right for the big guns, didn't I? Refactoring...

@jpetto jpetto force-pushed the bug-1243890-update-fxa-iframe-param-fx46 branch from ff9f3a2 to 55f5702 Feb 12, 2016
@jpetto
Copy link
Contributor Author

@jpetto jpetto commented Feb 12, 2016

Okay, I think I've stopped trying too hard. 😁

Ready for a real review.

@alexgibson
Copy link
Member

@alexgibson alexgibson commented Feb 12, 2016

When I updated the bedrock docs for local testing, I totally forgot to include the bit about using the right port (127.0.0.1:8111) - could you add it back here? 😄

@jpetto
Copy link
Contributor Author

@jpetto jpetto commented Feb 12, 2016

When I updated the bedrock docs for local testing, I totally forgot to include the bit about using the right port (127.0.0.1:8111) - could you add it back here? 😄

I think I can handle that. 👍

- Update FxA docs to include local server IP/port
@alexgibson
Copy link
Member

@alexgibson alexgibson commented Feb 12, 2016

Tested locally and both pages are working as expected with the correct iframe query params, and I see blue button after verifying email in FF 46+ 👍

r+wc from me once the docs are updated for my faux pas

Also noted that we should probably look to consolidate this code for the iframe sometime in the future, as there's a fair bit of duplication now?

@jpetto jpetto force-pushed the bug-1243890-update-fxa-iframe-param-fx46 branch from 55f5702 to 644e2ff Feb 12, 2016
alexgibson added a commit that referenced this pull request Feb 12, 2016
…aram-fx46

[fix bug 1243890] Update FxA iframe param for Fx46.
@alexgibson alexgibson merged commit 17dfdd9 into master Feb 12, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexgibson alexgibson deleted the bug-1243890-update-fxa-iframe-param-fx46 branch Feb 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants