-
Notifications
You must be signed in to change notification settings - Fork 798
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
Packages: Add a basic Jetpack Logo package #12379
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: June 4, 2019. |
.phpcs.xml.dist
Outdated
|
||
<rule ref="PHPCompatibilityWP"/> | ||
<rule ref="WordPress-Core" /> | ||
<rule ref="WordPress-Core"> | ||
<!-- Can we disable this just for `packages/*`? --> |
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 think it may be possible:
<!--
You can hard-code ignore patterns for specific sniffs,
a feature not available on the command line. Please note that
all sniff-specific ignore patterns are checked using absolute paths.
The code here will hide all messages from the Squiz DoubleQuoteUsage
sniff for files that match either of the two exclude patterns.
-->
<rule ref="Squiz.Strings.DoubleQuoteUsage">
<exclude-pattern>*/tests/*</exclude-pattern>
<exclude-pattern>*/data/*</exclude-pattern>
</rule>
-- https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset
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.
@tyxla wrote:
We have to support the previous couple of versions of WordPress, not just the latest version. We cannot depend on 5.6 support for ~6 months from now. It should be fine to bump to PHP 5.3 for now, as it includes all the things we need (right?) |
We got it to work, implemented changes to make it a full fledged singleton, added a common interfaces package for our packages to use, made the logo package implement it. We explored setting the common interfaces package as a dependency of the logo package instead of including it via top level composer.json, but haven't cracked that yet. |
7660f4d
to
f9e79a9
Compare
@gravityrail FWIW there hasn't been a PHP 5.2 to 5.3 minimum requirement bump in the WP core. It was directly bumped to 5.6. Also, I've removed the changes from this PR and rebased to the latest master, and it seems that since #12287 and #12395 we're no longer running tests against 5.3, so this is not something that this PR introduces. |
packages/logo/src/Logo.php
Outdated
return sprintf( | ||
'<img src="%s" class="jetpack-logo" alt="%s" />', | ||
esc_url( $this->url ), | ||
esc_attr__( |
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.
If this is going to be a dependency of Jetpack, then we should look into and elimitate accessibility issues as we spot them.
This is a pretty good example, where the alt
text provides different content as opposed to an actual alternative text to the Jetpack logotype.
packages/logo/src/Logo.php
Outdated
'<img src="%s" class="jetpack-logo" alt="%s" />', | ||
esc_url( $this->url ), | ||
esc_attr__( | ||
'Jetpack is a free plugin that utilizes powerful WordPress.com servers to enhance your site and simplify managing it', |
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.
'Jetpack is a free plugin that utilizes powerful WordPress.com servers to enhance your site and simplify managing it', | |
'Jetpack', |
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.
Is there a different attribute we could use to put the old string as extra information?
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.
Yeah, I'd very much like keeping the original string somehow, while making everything accessible. Could we use an aria attribute to sort that out @aldavigdis?
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 think this is more of a design issue. I suggest using a hyperlink wrapping the image tag somehow like this:
<a
href="https://www.jetpack.com/"
rel="external"
title="Visit the Jetpack website. Jetpack is a free plugin that utilizes powerful WordPress.com servers to enhance your site and simplify managing it."
>
<img src="foo.svg" alt="Jetpack." />
</a>
Notice that I added a reference to what the link does and a period after the end of the title and alt tags. This makes screen readers pause slightly.
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.
Since I prefer not making the logo a link unless we want to, I've gone with the other suggestion - just changed the alt
attribute to Jetpack.
.
I'll ask for a design review because of this change too, just to be sure we're good with it.
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.
Ah, I've also removed the localization function there, because we no longer need it - we don't translate Jetpack AFAIK.
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'd still wrap it with a __
function, just in case.
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 happy to do it if @Automattic/jetpack-crew are good with it - last time I did it, someone told me we aren't supposed to do it. Hence, I didn't do it here.
If you ask me, it's unnecessary - we don't localize our plugin name, and that should be a good enough reason not to localize this string.
e700669
to
b64ede1
Compare
This makes a great deal of sense. We don't need such a long description in this context. If we are thinking purely about screen readers then it might be good to describe the logo, in this context though, using it as a heading to announce the content block works for me. |
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 seem to be running into the same problem @Kraft described here:
#12379 (comment)
To reproduce:
rm -rf vendor
rm composer.lock
yarn build-production
8431c25
to
0a77889
Compare
See #12379 (comment) for more discussion about this.
Some more we can do to make it truly sing, but let's get this in as a public dress rehearsal. |
* Kick off the changelog * Add 7.3.1 * Update date and post link * changelog: add #12219 * changelog: add #12170 * changelog: add #12184 * Changelog: add #12268 * Changelog: add #12081 * Changelog: add #12323 * Changelog: add #12204 * Changelog: add #12269 * Changelog: add #12332 * changelog: add #12339 * changelog: add #12209 * Changelog: add #12319 * Changelog: add #12357 * Changelog: add #12124 * Changelog: add #12373 * Changelog: add #12252 * Changelog: add #12383 * Changelog: add #12372 * changelog: add #12337 * Changelog: add #12290 * Changelog: add #12301 * Changelog: add #12061 * Testing list: add instructions for #12061 * Changelog: add #12393 * Update minimum supported version See #12287 * Changelog: add #12406 * Testing list: add #12406 * Changelog: add #12277 * Changelog: add #12412 * Changelog: add #11318 * Changelog: add #12328 * Changelog: add #12425 * Changelog: add #12380 * Changelog: add #12428 * Changelog: add #12414 * Changelog: add #12395 * Changelog & Testing list: add #12416, #12417, #12418, and #12348 * changelog: add #12379 * Changelog: add #12341 * changelog: add #12444 * Changelog: add #12434 * Changelog: add #12454 * Changelog: add #12460 * Changelog: add #12463 * Changelog: add #12457 * Changelog / testing list: add #10333 * Changelog: add #12467 Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
This PR is an experiment to create and use a Jetpack Logo as a composer package. Similar to #12363, but I wanted to start it from scratch to go through the process, plus I introduced several changes, some of which are opinionated.
This is an initial attempt to address 3879-gh-jpop-issues.
I'm planning to use this PR for a base of several other experiments too. Feedback is appreciated.
Note on design review: The only design feedback I'm looking here is the alt attribute change we're doing here, in favor of better accessibility. See convo for details.
Changes proposed in this Pull Request:
alt
attribute to justJetpack.
- see convo/src
instead of/lib
, because it's more common for PHP libraries./logo
instead of/jetpack-logo
, becausejetpack
is assumed and therefore arbitrary.utils
in the namespace toassets
, because it's more specific.Logo
- Jetpack already exists everywhere, including in the namespace.composer install
to the Travis CI setup.php-build
script that basically runscomposer install
and add it to our build scripts.Is this a new feature or does it add/remove features to an existing part of Jetpack?
See p1HpG7-6V2-p2 and p1HpG7-6Ym-p2 for details.
Testing instructions:
yarn docker:phpunit --testsuite packages
Proposed changelog entry for your changes:
Notes
composer.lock
after releasing the logo as a package.