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 1131304 - Add share buttons to 'Download Firefox in your language' page #2774

Merged
merged 3 commits into from Mar 2, 2015
Merged

Fix Bug 1131304 - Add share buttons to 'Download Firefox in your language' page #2774

merged 3 commits into from Mar 2, 2015

Conversation

kyoshino
Copy link
Contributor

Here’s another optimization. Introducing the new site_header_share block to put the button in #masthead. The change will affect the existing /firefox/hello/ and /plugincheck/ pages.

This /firefox/all/ page has been localized into Japanese only and we have almost the same fallback text, so this is good to go.

I have added one more button for /firefox/developer/ because the change is small and similar. This time again, the existing copy will be used as the l10n fallback.

@alexgibson r?

@alexgibson alexgibson self-assigned this Feb 25, 2015
@alexgibson
Copy link
Member

We should probably also add {% block site_header_share %}{% endblock %} to bedrock/base/templates/base.html

@@ -1,5 +1,7 @@
{% extends "firefox/base-resp.html" %}

{% set num_languages = settings.CONTRIBUTE_NUMBERS.num_languages|round(-1, 'floor')|int %}
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... is this supposed to round 87 down to 80? Any reason not just go for a more exact figure if we're pulling the same data that's on both the homepage and /contribute?

@alexgibson
Copy link
Member

Ping @flodolo - new strings for share CTA on /firefox/all and /firefox/developer

html[dir="rtl"] #masthead h2 {
float: right;
margin: 0 auto;
width: @widthDesktop - @gridGutterWidth*2;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: (@gridGutterWidth * 2) for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

@alexgibson
Copy link
Member

This change looks good, and +1 to the standard #masthead styles for easy re-use 👍

This PR is an r+wc from me, but will hold off merging until @flodolo gives l10n approval. Thanks again @kyoshino!

@alexgibson
Copy link
Member

I don't imaging this will cause a conflict, but the only other thing we should check on before merging this is for any impact on the new Firefox Family Navigation that is currently in development. @jpetto - do you see any issues here or reason's to hold off merging this change? I assume it could only impact the Hello product page?

@flodolo
Copy link
Contributor

flodolo commented Feb 25, 2015

We're flooding localizers with requests :-\

How many of requests of this kind do we have still pending?

{% if channel == 'release' -%}
{% set share_urls = { 'twitter': 'http://mzl.la/1IMdR5T', 'googleplus': 'http://mzl.la/1y7TgOf', 'facebook': 'http://mzl.la/1ATF965' } -%}
{% if l10n_has_tag('share_cta') -%}
{% set share_text = _('Thanks to contributions of Mozilla community members around the world, Firefox is available in %s languages:')|format(num_languages) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not going to work for a lot of locales (languages need to be adapted to the actual number).

We had the same issue in the past, and ended up using "in more than X languages" (not sure what X is these days).

Copy link
Contributor

Choose a reason for hiding this comment

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

And now that I realize it, we're not actually localizing this page (maybe just for Japanese), but worth fixing anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then let's use a static number and I'll update the Japanese copy. X = 80 according to the bug.

@kyoshino
Copy link
Contributor Author

Solved issues @alexgibson and @flodolo noted.

@@ -13,7 +13,7 @@
{% elif channel == 'esr' %}
{{ _('Firefox ESR is intended for groups who deploy and maintain the desktop environment in large organizations. <a href="%s">Learn more.</a>')|format(url('firefox.organizations.organizations')) }}
{% else %}
{{ _('Firefox is available in more than %s languages thanks to the contributions of Mozilla community members from around the world.')|format('80') }}
{{ _('Firefox is available in more than 80 languages thanks to the contributions of Mozilla community members from around the world.') }}
Copy link
Member

Choose a reason for hiding this comment

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

If we're no longer changing this, why hard-code the value instead of allow us to change it in the future without breaking the string?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, i missed @flodolo's comment, never mind 👍

@alexgibson
Copy link
Member

Teeny nit: focus styling could be improved a little. I'm all for leaving a dotted focus outline, but it should probably not be cropped at the top/bottom.

share-focus

@jpetto
Copy link
Contributor

jpetto commented Feb 25, 2015

The Firefox Family nav will replace the entire {% block site_header %} block. This new nav will definitely be placed on /firefox/hello/, likely placed on /firefox/developer/, and possibly placed on /firefox/all/.

There's really no reason to hold off on merging this PR, though it's possible only 1 of the 4 pages updated here will retain the {% block site_header %} after 3/12. 😟

@alexgibson
Copy link
Member

There's really no reason to hold off on merging this PR, though it's possible only 1 of the 4 pages updated here will retain the {% block site_header %} after 3/12. 😟

Thanks for the heads-up @jpetto - sounds like we'll have to work out where to re-position the sharing widget on these pages after 3/12 😢

@kyoshino - could you please feed this info back to whoever is managing these social sharing updates? Many thanks.

@kyoshino
Copy link
Contributor Author

Then it might be better to postpone this change. Asked in the bug if it's okay.

@kyoshino
Copy link
Contributor Author

:williamr in the bug says we can wait until March.

@alexgibson
Copy link
Member

@kyoshino - as Jen said, we are happy to merge this PR and then revisit later this month. unless you would rather wait?

@kyoshino
Copy link
Contributor Author

kyoshino commented Mar 2, 2015

Then :shipit: 👍

alexgibson added a commit that referenced this pull request Mar 2, 2015
Fix Bug 1131304 - Add share buttons to 'Download Firefox in your language' page
@alexgibson alexgibson merged commit ed055df into mozilla:master Mar 2, 2015
@kyoshino kyoshino deleted the bug-1131304-firefox-all-share branch March 2, 2015 16:40
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

4 participants