Bug 970449 - Add a link to YouTube to the footer #1750

Merged
merged 1 commit into from Apr 17, 2014

Conversation

Projects
None yet
7 participants
@kyoshino
Member

kyoshino commented Mar 4, 2014

This PR introduces new strings, so the l10n needs an update. cc @flodolo

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Mar 5, 2014

Member

I'm seeing some breakage on certain pages (on quite a few of the /firefox pages, e.g. /os /fx, etc).

footer

Member

alexgibson commented Mar 5, 2014

I'm seeing some breakage on certain pages (on quite a few of the /firefox pages, e.g. /os /fx, etc).

footer

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Mar 5, 2014

Member

Please disregard my last comment, was my cache that needed clearing!

Member

alexgibson commented Mar 5, 2014

Please disregard my last comment, was my cache that needed clearing!

+ <div class="col col-2">
+ <ul class="links-join">
+ <li><a href="{{ url('mozorg.contact.spaces.spaces-landing') }}">{{ _('Contact Us') }}</a> &middot; <a href="{{ url('mozorg.partnerships') }}">{{ _('Partner with Us' ) }}</a></li>
+ <li><a href="{{ donate_url() }}?source=mozillaorg_footer&amp;amount=20" class="donate">{{ _('Donate') }}</a> &middot; <a href="https://affiliates.mozilla.org/">{{ _('Firefox Affiliates') }}</a></li>

This comment has been minimized.

@alexgibson

alexgibson Mar 5, 2014

Member

I really like the new layout, much cleaner and more concise! 👍

It's a nit, but I wonder if the semantics could be improved here slightly? We have a single list item with two links in it, separated by a &middot in the markup. It might be slightly more to rework, but maybe it would be cleaner to have each link as an inline list item?

Update - another thought, perhaps the first suggestion might be a bit too long-winded to warrant the extra markup and styling. I wonder if we could just add <span role="presentation">&middot;</span> to the separator?

@alexgibson

alexgibson Mar 5, 2014

Member

I really like the new layout, much cleaner and more concise! 👍

It's a nit, but I wonder if the semantics could be improved here slightly? We have a single list item with two links in it, separated by a &middot in the markup. It might be slightly more to rework, but maybe it would be cleaner to have each link as an inline list item?

Update - another thought, perhaps the first suggestion might be a bit too long-winded to warrant the extra markup and styling. I wonder if we could just add <span role="presentation">&middot;</span> to the separator?

This comment has been minimized.

@kyoshino

kyoshino Mar 6, 2014

Member

Good question! I have improved the markup as you suggested. 😺

@kyoshino

kyoshino Mar 6, 2014

Member

Good question! I have improved the markup as you suggested. 😺

@icaaq

This comment has been minimized.

Show comment
Hide comment
@icaaq

icaaq Mar 6, 2014

Contributor

The <span> element has no semantic value, so there is no need to add role="presentation" to it
http://www.w3.org/TR/wai-aria/roles#presentation

Contributor

icaaq commented Mar 6, 2014

The <span> element has no semantic value, so there is no need to add role="presentation" to it
http://www.w3.org/TR/wai-aria/roles#presentation

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 6, 2014

Member

We have 3 new strings: Mozilla on %s, Firefox on %s and YouTube. Twitter and Facebook are already there, and we could probably reuse the current strings: Mozilla on Twitter and Mozilla on Twitter. And looks like YouTube is not officially translated, though our localizers can later do that if needed. So I think we don't have to call for localizers here. @flodolo @pascalchevrel Thoughts?

Member

kyoshino commented Mar 6, 2014

We have 3 new strings: Mozilla on %s, Firefox on %s and YouTube. Twitter and Facebook are already there, and we could probably reuse the current strings: Mozilla on Twitter and Mozilla on Twitter. And looks like YouTube is not officially translated, though our localizers can later do that if needed. So I think we don't have to call for localizers here. @flodolo @pascalchevrel Thoughts?

+ </div>
+ <div class="col col-2">
+ <ul class="links-join">
+ <li class="link-spaces"><a href="{{ url('mozorg.contact.spaces.spaces-landing') }}">{{ _('Contact Us') }}</a></li>

This comment has been minimized.

@alexgibson

alexgibson Mar 11, 2014

Member

I wonder if we could use some more generic CSS class names here, so they're not quite as closely tied to the content should it change in the future?

@alexgibson

alexgibson Mar 11, 2014

Member

I wonder if we could use some more generic CSS class names here, so they're not quite as closely tied to the content should it change in the future?

This comment has been minimized.

@kyoshino

kyoshino Mar 11, 2014

Member

👍

+ </div>
+ <div class="col col-3">
+ <ul class="links-social">
+ <li class="links-mozilla">{{ _('Mozilla on %s')|format((['<a href="%s">%s</a>'] * 2)|join(' &middot; ')|format('https://twitter.com/mozilla', _('Twitter'), 'https://www.facebook.com/mozilla', _('Facebook'))|safe) }}</li>

This comment has been minimized.

@alexgibson

alexgibson Mar 11, 2014

Member

Could we eliminate the &middot; here as well maybe? Perhaps a simple comma , separator would do here considering this reads more like a typical sentence?

@alexgibson

alexgibson Mar 11, 2014

Member

Could we eliminate the &middot; here as well maybe? Perhaps a simple comma , separator would do here considering this reads more like a typical sentence?

This comment has been minimized.

@icaaq

icaaq Mar 11, 2014

Contributor

as discussed on IRC it would be great if the twitter and facebook links would make sense out of context.

@icaaq

icaaq Mar 11, 2014

Contributor

as discussed on IRC it would be great if the twitter and facebook links would make sense out of context.

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 11, 2014

Member

So I have modified the markup for a11y. The social link now looks like <li><a href="">Twitter<span> (@firefox)</span></a></li> and the <span> is visually hidden.

I think the &middot; here is okay (I have changed it as a CSS generated content though) as it consists with other links in the center column.

I still keep the string Mozilla on %s for l10n flexibility.

Member

kyoshino commented Mar 11, 2014

So I have modified the markup for a11y. The social link now looks like <li><a href="">Twitter<span> (@firefox)</span></a></li> and the <span> is visually hidden.

I think the &middot; here is okay (I have changed it as a CSS generated content though) as it consists with other links in the center column.

I still keep the string Mozilla on %s for l10n flexibility.

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 11, 2014

Member

Here's a screenshot when I use VoiceOver:
voiceover-links

Member

kyoshino commented Mar 11, 2014

Here's a screenshot when I use VoiceOver:
voiceover-links

@icaaq

This comment has been minimized.

Show comment
Hide comment
@icaaq

icaaq Mar 11, 2014

Contributor

A.some!

Contributor

icaaq commented Mar 11, 2014

A.some!

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 11, 2014

Member

Thanks!

Member

kyoshino commented Mar 11, 2014

Thanks!

@kyoshino

This comment has been minimized.

Show comment
Hide comment
Member

kyoshino commented Mar 14, 2014

@flodolo @pascalchevrel what do you think of #1750 (comment)?

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Mar 14, 2014

Contributor

Sorry, this approach is not going to work.

People may not translate the name "X", but they could need to adapt it to the case or just transliterate it.

Take a look at how "Firefox on Twitter" is translated and you'll get the picture (Transvision Beta searches also www.mozilla.org strings 👍 )
http://transvision-beta.mozfr.org/string/?entity=mozilla_org/main.lang:677a7b03&repo=mozilla_org

Contributor

flodolo commented Mar 14, 2014

Sorry, this approach is not going to work.

People may not translate the name "X", but they could need to adapt it to the case or just transliterate it.

Take a look at how "Firefox on Twitter" is translated and you'll get the picture (Transvision Beta searches also www.mozilla.org strings 👍 )
http://transvision-beta.mozfr.org/string/?entity=mozilla_org/main.lang:677a7b03&repo=mozilla_org

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 14, 2014

Member

Hmm, so do I have to provide a new string to include everything? I (and probably localizers) don't want to update the entire string every time a new social service (e.g. Google+) is added.

Mozilla on <a href="%(twitter)s">Twitter</a> ・ <a href="%(facebook)s">Facebook</a>
Firefox on <a href="%(twitter)s">Twitter</a> ・ <a href="%(facebook)s">Facebook</a> ・ <a href="%(youtube)s">YouTube</a>

Member

kyoshino commented Mar 14, 2014

Hmm, so do I have to provide a new string to include everything? I (and probably localizers) don't want to update the entire string every time a new social service (e.g. Google+) is added.

Mozilla on <a href="%(twitter)s">Twitter</a> ・ <a href="%(facebook)s">Facebook</a>
Firefox on <a href="%(twitter)s">Twitter</a> ・ <a href="%(facebook)s">Facebook</a> ・ <a href="%(youtube)s">YouTube</a>

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 14, 2014

Member

Or can we just avoid using "on" after "Mozilla" and "Firefox" like this?

{{ _('Mozilla') }}: <a href="{{ twitter }}">{{ _('Twitter') }} ・ <a href="{{ facebook }}">{{ _('Facebook') }}
{{ _('Firefox') }}: <a href="{{ twitter }}">{{ _('Twitter') }} ・ <a href="{{ facebook }}">{{ _('Facebook') }} ・ <a href="{{ youtube }}">{{ _('YouTube') }}

Member

kyoshino commented Mar 14, 2014

Or can we just avoid using "on" after "Mozilla" and "Firefox" like this?

{{ _('Mozilla') }}: <a href="{{ twitter }}">{{ _('Twitter') }} ・ <a href="{{ facebook }}">{{ _('Facebook') }}
{{ _('Firefox') }}: <a href="{{ twitter }}">{{ _('Twitter') }} ・ <a href="{{ facebook }}">{{ _('Facebook') }} ・ <a href="{{ youtube }}">{{ _('YouTube') }}

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 20, 2014

Member

Maybe the last one (removing "on") is better in terms of l10n and maintainability?

Member

kyoshino commented Mar 20, 2014

Maybe the last one (removing "on") is better in terms of l10n and maintainability?

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Mar 20, 2014

Contributor

How often do you see us adding new services? I hope it's not that frequent. We currently have almost 1500 strings on mozilla.org, I wouldn't consider it a burden to add a couple of strings from now and then for new services.

In general: hard-coding positions (name: service), or characters (: ・), is not a good choice.

I know it's going to be a silly question, but why can't we use logos+alt/title localized text? Like
Mozilla: [T] [F]
Firefox: [T] [F] [Y]

Logos copyright issues, accessibility, unwanted distracting images in the footer?

Contributor

flodolo commented Mar 20, 2014

How often do you see us adding new services? I hope it's not that frequent. We currently have almost 1500 strings on mozilla.org, I wouldn't consider it a burden to add a couple of strings from now and then for new services.

In general: hard-coding positions (name: service), or characters (: ・), is not a good choice.

I know it's going to be a silly question, but why can't we use logos+alt/title localized text? Like
Mozilla: [T] [F]
Firefox: [T] [F] [Y]

Logos copyright issues, accessibility, unwanted distracting images in the footer?

@m-alexis

This comment has been minimized.

Show comment
Hide comment
@m-alexis

m-alexis Mar 24, 2014

Contributor

Flod, we would need significant review and discussion with the Creative team re: logos in the footer. Let's move forward with what we currently have and we can iterate on that if necessary.

Contributor

m-alexis commented Mar 24, 2014

Flod, we would need significant review and discussion with the Creative team re: logos in the footer. Let's move forward with what we currently have and we can iterate on that if necessary.

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 25, 2014

Member

@flodolo Folks in the bug have chosen text over icons, not to promote their services nor break their logo guidelines. Accessibility is also considered. Those strings won't be updated frequently, but there's still a chance.

The position is not hard-coded; localizers can alter _('Mozilla on %s') or _('Mozilla: %s') if needed. And &middot; might be a language-neutral separator? I see it on Twitter profiles.

Member

kyoshino commented Mar 25, 2014

@flodolo Folks in the bug have chosen text over icons, not to promote their services nor break their logo guidelines. Accessibility is also considered. Those strings won't be updated frequently, but there's still a chance.

The position is not hard-coded; localizers can alter _('Mozilla on %s') or _('Mozilla: %s') if needed. And &middot; might be a language-neutral separator? I see it on Twitter profiles.

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Mar 25, 2014

Contributor

Mozilla/Firefox on %s

I don't see this working: let's suppose you add this string and _('Twitter'), this needs to be adapted to the grammar case for "on something" (ablative). This will prevent us from having "Twitter" used as a name (nominative).

If you want to use a variable in this context, I guess the only way is what you proposed a couple of comments above

{{ _('Mozilla :') }} <a href="{{ twitter }}">{{ _('Twitter') }} ・ <a href="{{ facebook }}">{{ _('Facebook') }}

@pascalchevrel
Opinions on this?

Contributor

flodolo commented Mar 25, 2014

Mozilla/Firefox on %s

I don't see this working: let's suppose you add this string and _('Twitter'), this needs to be adapted to the grammar case for "on something" (ablative). This will prevent us from having "Twitter" used as a name (nominative).

If you want to use a variable in this context, I guess the only way is what you proposed a couple of comments above

{{ _('Mozilla :') }} <a href="{{ twitter }}">{{ _('Twitter') }} ・ <a href="{{ facebook }}">{{ _('Facebook') }}

@pascalchevrel
Opinions on this?

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Mar 27, 2014

Member

Changed the strings as @flodolo suggested. Does this work?

Member

kyoshino commented Mar 27, 2014

Changed the strings as @flodolo suggested. Does this work?

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Mar 27, 2014

Contributor

Yes, this works. Let me know when/if I can extract these strings to start the localization process (we'll need to wait for a good coverage before pushing it to prod).

Contributor

flodolo commented Mar 27, 2014

Yes, this works. Let me know when/if I can extract these strings to start the localization process (we'll need to wait for a good coverage before pushing it to prod).

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Apr 3, 2014

Member

@flod - I think we're good here with the strings now, so please begin l10n

Member

alexgibson commented Apr 3, 2014

@flod - I think we're good here with the strings now, so please begin l10n

media/css/sandstone/sandstone.less
@@ -1,3 +1,4 @@
+@import "lib.less";

This comment has been minimized.

@alexgibson

alexgibson Apr 3, 2014

Member

I don't think this should need lib.less, as much of the content is already provided in variables.less and would be duplicated.

@alexgibson

alexgibson Apr 3, 2014

Member

I don't think this should need lib.less, as much of the content is already provided in variables.less and would be duplicated.

This comment has been minimized.

@kyoshino

kyoshino Apr 3, 2014

Member

OK, I'll remove it.

@kyoshino

kyoshino Apr 3, 2014

Member

OK, I'll remove it.

+ <li>
+ {{ _('Mozilla:') }}
+ <ul>
+ <li><a href="https://twitter.com/mozilla">{{ _('Twitter') }}<span> (@mozillla)</span></a></li>

This comment has been minimized.

@craigcook

craigcook Apr 10, 2014

Member

three ls in "mozillla"... should fix that :)

@craigcook

craigcook Apr 10, 2014

Member

three ls in "mozillla"... should fix that :)

This comment has been minimized.

@sgarrity

sgarrity Apr 10, 2014

Contributor

Or we could keep the three Ls in "mozillla" and add a third T to "twittter".

@sgarrity

sgarrity Apr 10, 2014

Contributor

Or we could keep the three Ls in "mozillla" and add a third T to "twittter".

This comment has been minimized.

@kyoshino

kyoshino Apr 10, 2014

Member

oops 😸

@kyoshino

kyoshino Apr 10, 2014

Member

oops 😸

This comment has been minimized.

@kyoshino

kyoshino Apr 10, 2014

Member

Then faceboook!

@kyoshino

kyoshino Apr 10, 2014

Member

Then faceboook!

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Apr 10, 2014

Member

@flodolo Those new strings are basically service names like Twitter or Facebook, so could we get this merged without waiting further l10n?

Member

kyoshino commented Apr 10, 2014

@flodolo Those new strings are basically service names like Twitter or Facebook, so could we get this merged without waiting further l10n?

@flodolo

This comment has been minimized.

Show comment
Hide comment
@flodolo

flodolo Apr 10, 2014

Contributor

Yes, you can merge this. I'll discuss with Pascal if we want to eventually expose all or just some of these strings.

Contributor

flodolo commented Apr 10, 2014

Yes, you can merge this. I'll discuss with Pascal if we want to eventually expose all or just some of these strings.

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Apr 10, 2014

Member

Thanks @flodolo. I just fixed the typos so it's ready to be merged?

Member

kyoshino commented Apr 10, 2014

Thanks @flodolo. I just fixed the typos so it's ready to be merged?

+ }
+
+ span {
+ .visually-hidden; /* for a11y */

This comment has been minimized.

@alexgibson

alexgibson Apr 14, 2014

Member

This is throwing an error when less is compiling: NameError: .visually-hidden is undefined

@alexgibson

alexgibson Apr 14, 2014

Member

This is throwing an error when less is compiling: NameError: .visually-hidden is undefined

This comment has been minimized.

@kyoshino

kyoshino Apr 15, 2014

Member

Ah oh... .visually-hidden is missing in sandstone.less as it's in lib.css. I have just copied it to mixins.less so it should work now. Tested with the /firefox/features/ page.

@kyoshino

kyoshino Apr 15, 2014

Member

Ah oh... .visually-hidden is missing in sandstone.less as it's in lib.css. I have just copied it to mixins.less so it should work now. Tested with the /firefox/features/ page.

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Apr 17, 2014

Member

Thanks for the update @kyoshino, but looks like this one needs a rebase.

Member

alexgibson commented Apr 17, 2014

Thanks for the update @kyoshino, but looks like this one needs a rebase.

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Apr 17, 2014

Member

Thanks! Just rebased.

Member

kyoshino commented Apr 17, 2014

Thanks! Just rebased.

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Apr 17, 2014

Member

Tested with the /firefox/features/ page.

Looks like the new footer isn't spanning the full width of the page on non-responsive pages like this one? ^

Member

alexgibson commented Apr 17, 2014

Tested with the /firefox/features/ page.

Looks like the new footer isn't spanning the full width of the page on non-responsive pages like this one? ^

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Apr 17, 2014

Member

This is a screenshot on /firefox/channel/ (with the lang switcher). I guess it's a proper layout, almost the same as responsive pages.

screen shot 2014-04-05 at 14 31 21

Member

kyoshino commented Apr 17, 2014

This is a screenshot on /firefox/channel/ (with the lang switcher). I guess it's a proper layout, almost the same as responsive pages.

screen shot 2014-04-05 at 14 31 21

@alexgibson

This comment has been minimized.

Show comment
Hide comment
@alexgibson

alexgibson Apr 17, 2014

Member

Gah, the cache hit me again - sorry @kyoshino I needed to flush my css files. Looking good now 👍

Member

alexgibson commented Apr 17, 2014

Gah, the cache hit me again - sorry @kyoshino I needed to flush my css files. Looking good now 👍

alexgibson added a commit that referenced this pull request Apr 17, 2014

Merge pull request #1750 from kyoshino/bug-970449-footer-youtube
Bug 970449 - Add a link to YouTube to the footer

@alexgibson alexgibson merged commit 451cb6d into mozilla:master Apr 17, 2014

1 check passed

default Jenkins build 'bedrock_github' #3840 has succeeded
Details

@kyoshino kyoshino deleted the kyoshino:bug-970449-footer-youtube branch Apr 17, 2014

@kyoshino

This comment has been minimized.

Show comment
Hide comment
@kyoshino

kyoshino Apr 17, 2014

Member

Thanks! 😸

Member

kyoshino commented Apr 17, 2014

Thanks! 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment