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 1246795 - Firefox Accounts home promo #3845

Merged

Conversation

craigcook
Copy link
Member

Also fixes bug 1247103 - replace Webmaker promo with Teach promo

The Accounts promo is English only, no l10n (for now) so I haven't wrapped the strings. Other locales fall back to the evergreen private browsing promo.

The Teach promo we had previously and I believe the strings are still in place behind the webmaker_promo_introducing flag. @flodolo please correct me if I'm wrong and I can give it a new flag for the new string.

@flodolo flodolo added the L10N label Feb 10, 2016
@flodolo
Copy link
Contributor

flodolo commented Feb 10, 2016

All the old strings have been removed, this is all we got.

This is basically a new string, so we need a different tag. Also, I'd suggest to remove the previous webmaker tag and string to avoid starting to pile up again.

Side question: if we plan to localize the FxA tile at some point, it would be better to expose the strings now (not sure what's up with all the en-US only tiles recently).

@craigcook
Copy link
Member Author

Ah, looks like I had outdated lang files, I still have the old strings locally. I'll give it a new tag.

The FxA promo links to the accounts page which isn't currently localized. If that page gets localized we could localize the promo as well, but I wouldn't want to link a localized promo to an English page. Can we expose the strings without actually activating the promo?

@craigcook craigcook force-pushed the bug-1246795-fxaccounts-home-promo branch 3 times, most recently from 0339cd5 to 3e77dd8 Compare February 10, 2016 22:43
@craigcook
Copy link
Member Author

New flag for the Teach promo: teach_the_web_promo

And I've wrapped strings for the FxA promo and put it behind the flag firefox_accounts_promo, but it's also explicitly behind an "if English" check. If we do want to release it to more locales down the line we can remove the English check (assuming the accounts page is also localized).

@flodolo
Copy link
Contributor

flodolo commented Feb 11, 2016

OK, that makes sense. Leaving the strings wrapped is OK, at this point I'd remove the tag (we'd still need to remove the "if English" conditional is we decide to localize it).

We could drop the conditional and leave the tag, and I won't expose the strings to localizers, but the promo would show up on the dev server and it would be more confusing.

@flodolo
Copy link
Contributor

flodolo commented Feb 11, 2016

Added string and imported from the old file where possible
mozilla-l10n/www.mozilla.org@1271824

I'll clean up the old string when this is merged to production.

@@ -0,0 +1,3 @@
<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" [
<!ENTITY ns_flows "http://ns.adobe.com/Flows/1.0/">

Choose a reason for hiding this comment

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

Do we need the DOCTYPE etc. I reckon we can remove everything to just before the open <svg....

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, surprised the optimizer left this in... it certainly seems unnecessary. I'll update.

@schalkneethling
Copy link

Just one comment about the webmaker SVG. Other than that, and the suggestion from @flodolo to remove the tag and just keep the en conditional, the code looks good.

Fix bug 1247103 - replace Webmaker promo with Teach promo
@craigcook craigcook force-pushed the bug-1246795-fxaccounts-home-promo branch from 3e77dd8 to 04715e9 Compare February 11, 2016 17:08
@craigcook
Copy link
Member Author

Removed the firefox_accounts_promo l10n flag and SVG cruft. I think we're all set.

@schalkneethling
Copy link

r+ ☀️

schalkneethling pushed a commit that referenced this pull request Feb 11, 2016
…romo

Fix bug 1246795 - Firefox Accounts home promo
@schalkneethling schalkneethling merged commit 5467c4c into mozilla:master Feb 11, 2016
@craigcook craigcook deleted the bug-1246795-fxaccounts-home-promo branch February 11, 2016 17:31
@flodolo flodolo removed the L10N label Feb 17, 2016
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.

None yet

3 participants