Skip to content

[fix bug 1190716] Update Hello FTU for Firefox 40 to interactive version#3198

Merged
jpetto merged 1 commit intomozilla:masterfrom
alexgibson:bug-1190716-update-hello-ftu-interactive
Sep 3, 2015
Merged

[fix bug 1190716] Update Hello FTU for Firefox 40 to interactive version#3198
jpetto merged 1 commit intomozilla:masterfrom
alexgibson:bug-1190716-update-hello-ftu-interactive

Conversation

@alexgibson
Copy link
Copy Markdown
Contributor

No description provided.

@alexgibson
Copy link
Copy Markdown
Contributor Author

Ping @flodolo - as per comments in the bug, I believe we now have the final two new strings needed for this PR. See l10n tag hello_ftu_firefox_40_interactive

@flodolo
Copy link
Copy Markdown
Collaborator

flodolo commented Aug 14, 2015

Looks good to me, even if it's getting harder to follow all the code changes :-)

We have 2 new strings behind hello_ftu_firefox_40_interactive, it's probably easier to check after merge and push to prod how many are actually unused.

@alexgibson
Copy link
Copy Markdown
Contributor Author

We have 2 new strings behind hello_ftu_firefox_40_interactive, it's probably easier to check after merge and push to prod how many are actually unused.

Sounds good - I'll file a follow-up bug to this one after this goes out, and we'll see if we can clean up the tags in this page 👍

@alexgibson alexgibson removed the WIP label Aug 24, 2015
@alexgibson
Copy link
Copy Markdown
Contributor Author

Ok I think this should be good for review.

@jpetto r? (I think you've done the Hello dance before, so you're probably the most familiar?)

Testing notes:

There is no new functionality here other than a few updated strings and bits in the template. It is basically the same as what we shipped prior to PR #3115, but with the code refactored for new tests.

@jpetto jpetto self-assigned this Aug 24, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this operation expensive? Is it feasible to cache the result of this check so it only has to be executed once? I suppose the user could move the icon between page load and initializing the tour...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this gets called at multiple times in the tour depending on the state, so it's there in the event that the user could remove the Hello button at any time. It's only really costly in that it requires firing a custom event each time, but I think it's better to pay that cost here.

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Aug 24, 2015

Tour worked well in Fx 40. One question in hello-ftu.js, but not a blocker.

r+ from me, but looks like there's still discussion going on in the bug.

@alexgibson
Copy link
Copy Markdown
Contributor Author

Thanks @jpetto - holding off merging this until folks decide what to do in the bug. Should hopefully be resolved soon one way or another. Will ping here if it requires any code updates (@flodolo - really sorry if this ends up being another string change 😢).

@alexgibson
Copy link
Copy Markdown
Contributor Author

Ok, we're now unblocked on merging this one. There's been one last string change (cc @flodolo or @pascalchevrel), but we're now good to expose these to localizers.

@jpetto - I added a new commit for the string change. If all looks good, I'll squash and we can finally get this one out the door.

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Sep 3, 2015

String change looks good. Looks like we're ready to merge after a squash?

@alexgibson
Copy link
Copy Markdown
Contributor Author

Thanks @jpetto - squashed!

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Sep 3, 2015

Waiting until all the iOS stuff is out the door and solid before pushing more changes. Plan to merge either late 9/3 or 9/4.

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Sep 3, 2015

As iOS is delayed, and will be behind waffle switches, we should be safe to merge this now. 🎈

jpetto added a commit that referenced this pull request Sep 3, 2015
…-interactive

[fix bug 1190716] Update Hello FTU for Firefox 40 to interactive version
@jpetto jpetto merged commit 6e423c6 into mozilla:master Sep 3, 2015
@flodolo flodolo removed the L10N label Sep 7, 2015
@alexgibson alexgibson deleted the bug-1190716-update-hello-ftu-interactive branch January 28, 2016 15:38
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.

3 participants