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 Firefox Klar wordmark #932

Merged
merged 1 commit into from
May 1, 2024
Merged

Add Firefox Klar wordmark #932

merged 1 commit into from
May 1, 2024

Conversation

reemhamz
Copy link
Contributor

@reemhamz reemhamz commented Apr 30, 2024

Description

We do not have a CSS class defined for Firefox Klar wordmark to be used for German locales in Bedrock. Instead, we've been using the English Firefox Focus wordmark. This PR adds that classname so it can be used (with a {% if LANG=='de' %} conditional.

i.e. (example used from @janbrasna's PR in Bedrock:

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

Note

I didn't need to add a new logo component to support Klar since it's just Firefox Focus in German, so it uses the same logo.

Describe what this change does.

  • Imports the Firefox Klar wordmark
  • Creates a class for Firefox Klar to be used in Bedrock (mzp-t-product-klar in conjunction with mzp-c-wordmark)
  • I have recorded this change in CHANGELOG.md.

Issue

Fixes #930
Refs mozilla/bedrock#14490

Testing

http://localhost:3000/components/detail/wordmark--default -- change the firefox string to klar and you should see the wordmark appear on the page

@reemhamz reemhamz added Review: XS Code review time: 30 mins or less Needs:Review 👋 Ready for Developer Review labels Apr 30, 2024
@stephaniehobson stephaniehobson changed the title Add Firefox Klar watermark Add Firefox Klar wordmark Apr 30, 2024
@stephaniehobson stephaniehobson self-assigned this Apr 30, 2024
@stephaniehobson stephaniehobson self-requested a review April 30, 2024 19:35
Copy link
Contributor

@stephaniehobson stephaniehobson left a comment

Choose a reason for hiding this comment

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

R+

$dir: 'firefox/browser/klar';

@each $layout-size, $logo-size in wordmark.$logo-sizes {
@include wordmark.wordmark($class, $dir, $layout-size, $logo-size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: Is there any benefit to including the klar classes in the focus file? Are we likely to use one without the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Benefit? Probably, but I didn't want to mess up how the CSS variables are structured for wordmarks! I'm also not sure I know how to set up conditional logic to extract the Klar workmark only in German only through this file. Because we import one wordmark per file, we'd make to use more specification in how we import this into Bedrock's CSS file, too.

I'm happy to merge for now. Let me know if you think there's a way to cleanly do the above and we can implement that in another PR

@reemhamz reemhamz merged commit 56eb994 into main May 1, 2024
2 checks passed
@reemhamz reemhamz deleted the add-klar-watermark branch May 1, 2024 07:31
@@ -5,6 +5,7 @@ Product classes:
- `mzp-t-product-developer`
- `mzp-t-product-nightly`
- `mzp-t-product-focus`
- `mzp-t-product-klar`
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI there's the same list also in wordmark/readme.md that could benefit from this addition for consistency, if you think it's worth it.

I'm actually thinking about how to document properly that the Klar component is only among the wordmarks, and trying to use the product class with another branding element will fail and folks have to remember to use Focus classes for all other branding needs — so this is an odd duck, existing only for the text difference here, and not elsewhere where it has no use. Some kind of a note to warn people of how specific it is?

Xcopz1

This comment was marked as spam.

Xcopz1

This comment was marked as spam.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs:Review 👋 Ready for Developer Review Review: XS Code review time: 30 mins or less
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firefox Klar [DE] logo classes missing
4 participants