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

Bump Protocol to v19.2.0 and add l10n for Klar (Focus) branding #14490

Merged
merged 5 commits into from
Jun 19, 2024

Conversation

janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Apr 22, 2024

One-line summary

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

Significant changes and points to review

Issue / Bugzilla link

Fixes #14489

Testing

(either make build or make preflight depending on how you manage deps)
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:

media/css/firefox/browsers/mobile.scss Outdated Show resolved Hide resolved
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

@janbrasna janbrasna Apr 22, 2024

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…)

EDIT: removed for now, git tracks that as rename/mv, it's just not obvious in the nonl10n folder there's the last screenshot elsewhere in l10n media, but that's as designed… 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw, pages that switch out images best on language should have said images in the /img/l10n so the back-end logic switches them out, so you correctly placed them here 👍🏼

@reemhamz reemhamz added the Needs Review Awaiting code review label Apr 23, 2024
@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.

@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
@stephaniehobson stephaniehobson added the Needs Review Awaiting code review label May 29, 2024
@reemhamz
Copy link
Contributor

This PR is just waiting on the new Protocol version, set to publish soon: mozilla/protocol#937

@reemhamz
Copy link
Contributor

reemhamz commented Jun 17, 2024

The Protocol bump we've been waiting on is almost merged to Bedrock! Once that's done, feel free to continue on with this change :D

For now, I'm going to remove the "Needs Review" label. @janbrasna go ahead and add back the label once this PR finished :)

@reemhamz reemhamz removed the Needs Review Awaiting code review label Jun 17, 2024
janbrasna and others added 3 commits June 17, 2024 15:02
@janbrasna janbrasna marked this pull request as ready for review June 17, 2024 13:33
Copy link
Contributor

@reemhamz reemhamz left a comment

Choose a reason for hiding this comment

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

Thanks for your patience in waiting for the new Protocol version to get published!

I reviewed your work, and well done! This looks like it's good to go. r+ 🍉

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

Choose a reason for hiding this comment

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

Fwiw, pages that switch out images best on language should have said images in the /img/l10n so the back-end logic switches them out, so you correctly placed them here 👍🏼

@reemhamz reemhamz changed the title Add l10n for Klar (Focus) branding Bump Protocol to v19.2.0 and add l10n for Klar (Focus) branding Jun 18, 2024
@reemhamz reemhamz merged commit 586a89a into mozilla:main Jun 19, 2024
5 checks passed
@janbrasna janbrasna deleted the fix/focus-klar-l10n branch June 19, 2024 02:17
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