-
Notifications
You must be signed in to change notification settings - Fork 2k
Jetpack Logo: support light/dark variants, clean up markup #103279
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
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
e5526f1
to
0c67b2b
Compare
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~337 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~4866 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~1227 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
/** | ||
* Theme of the logo. | ||
* - `default`: use the default theme (ie. inherit). | ||
* - `light`: render. | ||
* - `dark`: use the dark theme. | ||
* @default 'default' | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand why we need a theme
prop instead of just using currentColor
for the "foreground" parts (with the "cutout" being transparent in the monochrome case). Is that part of what we're figuring out with the brand designers right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The monochrome version having a cutout vs the brand color version being filled white is part of the logo specs (see TTzDk3dzct551hYWRXPFa3-fi-2395_14811 ):
- branded color, light theme: white triangles, green circle, black text
- branded color, dark theme: white triangles, green circle, white text
- monochrome, light or dark: cutout mask, currentColor circle, currentColor text
This is a requirement regardless of the "using hardcoded white/black" question.
In other words, I added the "theme" prop to generate all possible combinations from the specs. Especially today, we need a way to render the logo accordingly on a light/dark background following those specs. And I believe it would apply to most (if not all) logos.
When we add a Theme
component, we may soft-deprecate the prop and recommend wrapping the logos in a Theme
component instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't use currentColor text in the branded color version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not sure if we can ditch the hardcoded white/black for the "Jetpack" text part in favour of always using currentColor
, that's something to discuss with brand designers (@jameskoster is on it, although maybe a direct ping to @keoshi could be quicker?).
If it were possible to always use currentColor
for the "Jetpack" text part, we could do without a theme
prop, but we'd need to hardcode the triangles to #fff
— which potentially could go out of sync with the color of the Jetpack text
|
||
export const JetpackLogo = ( { | ||
full = false, | ||
monochrome = false, | ||
theme = 'default', | ||
size = 32, | ||
className, | ||
...props | ||
}: JetpackLogoProps & React.SVGProps< SVGSVGElement > ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we normalize the other logo components, it might be good to define a shared TS interface (e.g. A8CLogoProps
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, alghough I'd say a separate follow-up task
@@ -23,7 +22,6 @@ export default function A4APluginsJetpackBanner() { | |||
description={ translate( | |||
'To manage plugins, Jetpack must be activated on each site. Your Pressable plan includes Jetpack Complete for free. Activate it to access plugin management in this dashboard.' | |||
) } | |||
icon={ <JetpackLogo size={ 16 } /> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jetpack logo here was ignored in the underlying Banner
component
color: var(--studio-gray-20); | ||
fill: currentColor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switching to color
+ fill
for a more comprehensive way to change the logo's color that is backwards compatible, but also works with the new version of the jetpack logo
@@ -346,7 +346,7 @@ | |||
body.is-section-stepper, | |||
body.is-section-signup { | |||
&:not(:has(.step-container-v2)) { | |||
svg:not(.jetpack-logo, .social-buttons__button svg) { | |||
svg:not(.step-container__jetpack-powered-logo, .social-buttons__button svg) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using .step-container__jetpack-powered-logo
instead of the internal .jetpack-logo
class
iconComponent={ | ||
<JetpackLogo monochrome className="quick-links__action-box-icon" /> | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the same Jetpack logo as other jetpack-related quick links, instead of a slightly different jetpack logo
fill: currentColor; | ||
color: var(--color-neutral-60); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before — ie. apply the same fill color to both existing SVGs and also the new Jetpack color
Let's wait for a response re. having to use hardcoded white/black vs using currentColor (see conversation) before moving forward, as it may allow us to do without an additional |
Part of #
Proposed Changes
Apply some of the changes attempted in #103029, but trying to aligin better to the plan proposed in Linear DOTCOM-12752
Why are these changes being made?
Testing Instructions
Kapture.2025-05-09.at.12.58.18.mp4
Pre-merge Checklist