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

Add l10n for Klar (Focus) branding #14490

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

janbrasna
Copy link
Contributor

One-line summary

Enables l10n for Focus screenshot on landing page, and locally adds a missing Protocol class for the wordmark.

Significant changes and points to review

(see notes below)

Issue / Bugzilla link

Fixes #14489

Testing

http://localhost:8000//de/firefox/browsers/mobile/focus/
http://localhost:8000/de/firefox/browsers/mobile/

Copy link
Contributor Author

@janbrasna janbrasna left a comment

Choose a reason for hiding this comment

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

Notes for reviewer:

Comment on lines +57 to +60
// define '.mzp-t-product-klar' until https://github.com/mozilla/protocol/issues/930
&.mzp-t-product-focus.mzp-t-product-klar {
background-image: url('/media/protocol/img/logos/firefox/browser/klar/logo-word-hor.svg');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've opened an issue upstream:

that is not too hard to resolve, so once/if confirmed as valid and not intentional wontfix, I might do the fix there, but until resolved, released and updated here, this is the least painful shim.

srcset={
'img/firefox/browsers/mobile/index/firefox-focus-high-res.jpg': '2x'
'img/firefox/mobile/firefox-focus-high-res.jpg': '2x'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have left the default imgs in their original location too for now, but wondering what's the best practice? Remove them and keep only in l10/en-US? (I'd ideally symlink them, from img/l10n/en-US/* to img/firefox/browsers/*, but not sure how the tooling likes that…)

@reemhamz
Copy link
Contributor

Thank you for bringing this to light!!!
I think the best course of action would be to make this change in Protocol itself, and I managed to do that to save you some time 👍🏼 Once that gets reviewed and merged, we can update this PR to use the updated version of Protocol and have Firefox Klar's wordmark be usable :)

I suggest you convert this PR to draft for now, and we can update it when the Protocol changes come in.

@@ -57,7 +57,7 @@
media_include='firefox/browsers/mobile/includes/s2d-focus.html',
media_after=True
) %}
<div class="mzp-c-wordmark mzp-t-wordmark-md mzp-t-product-focus">{{ ftl('mobile-focus-firefox-focus') }}</div>
<div class="mzp-c-wordmark mzp-t-wordmark-md mzp-t-product-focus{% if LANG=='de' %} mzp-t-product-klar{% endif %}">{{ ftl('mobile-focus-firefox-focus') }}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

To Do: If this is waiting for mozilla/protocol#932 before merging, please don't put both mzp-t-product-focus and mzp-t-product-klar on the element together. Focus could over-write klar if the includes are in the wrong order.

Suggested change
<div class="mzp-c-wordmark mzp-t-wordmark-md mzp-t-product-focus{% if LANG=='de' %} mzp-t-product-klar{% endif %}">{{ ftl('mobile-focus-firefox-focus') }}</div>
<div class="mzp-c-wordmark mzp-t-wordmark-md {% if LANG=='de' %} mzp-t-product-klar{% else %}mzp-t-product-focus{% endif %}">{{ ftl('mobile-focus-firefox-focus') }}</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

ah good catch!

@janbrasna janbrasna marked this pull request as draft April 30, 2024 22:29
@reemhamz reemhamz removed the Needs Review Awaiting code review label May 1, 2024
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.

Klar branding shown as Focus on DE images
3 participants