-
Notifications
You must be signed in to change notification settings - Fork 2k
BigSky logo: adhere to the common logo specs #103612
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
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 |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Async-loaded Components (~137 bytes removed 📉 [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. |
1df3a47
to
63c5f4f
Compare
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.
LGTM
const brandFill = monochrome ? 'currentColor' : '#030FB0'; | ||
const isMasked = monochrome; | ||
|
||
const trimmedTitle = title.trim(); |
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.
Why do we trim()
? I feel like we should expect consumers to pass a well-formed string. And if this is part of the truthiness checks, I'd rather we handle this as a properly-empty nullish value rather than an empty string.
Also, is the idea with a falsey title
that it'd be used as a decorative image? And if so, would we also need to hide it from assistive technology (e.g. aria-hidden
, role=presentation
) ? Or should we just not support the falsey title altogether?
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.
Good points.
- I removed the
trim()
call, but kepttitle
as a pure string - I added more explicit documentation about using
aria-hidden
as the recommended way to remove from assistive technology
This approach follows the specs that from DS-216 (which originated from this conversation), although I'm happy to revisit the specs if we think they could be improved
* BigSky logo: remove normalized, fix title, mask if monochrome, expose Mark component * Remove unnecessary Storybook stories * CHANGELOG * Do not `trim()` the title * Add more explicit docs around using `aria-hidden` to remove from assistive tech
Follow-up to #103546 (comment)
Related to DS-216
Proposed Changes
Tweak the
BigSkyLogo.Mark
logo props in order to follow the latest specs (defined in DS-216):normalized
variant;aria-labelledby
to the SVG element pointing to thetitle
;path
by running the SVG through SVGOMGref
Why are these changes being made?
To stick to the logo API specs (see DS-216)
Testing Instructions
Load Storybook, make sure:
trunk
(apart from the lack ofnormalized
);aria-labelledby
prop when the title is defined;monochrome="true"
In Calypso:
Pre-merge Checklist