Packages: Add a basic Jetpack Logo package by tyxla · Pull Request #12379 · Automattic/jetpack

tyxla · 2019-05-15T12:48:51Z

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:

Same as Add logo as dependent package #12363, but has several more changes:
Design changes: Changed the original alt attribute to just Jetpack. - see convo
Source directory renamed to /src instead of /lib, because it's more common for PHP libraries.
Package dir renamed to /logo instead of /jetpack-logo, because jetpack is assumed and therefore arbitrary.
Removed version from the namespace, because I think it can cause more problems than it would actually solve (also see this discussion: Composer? #12213 (comment)).
Rename utils in the namespace to assets, because it's more specific.
Rename class to just Logo - Jetpack already exists everywhere, including in the namespace.
Fixed the license - now it's the same as Jetpack, the plugin.
Refactored a bit to make easier to test and more flexible.
Move the actual logo file to the package.
Allow for custom logo URL in the class interface.
Add a README with example usage.
Ignore composer.lock in packages
Add packages directory to the phpcs whitelist for linting.
Add some temporary phpcs ignores - in the long term, we need to make PSR-4 and WP coding standards compatible.
Added basic tests for the new package.
Add composer install to the Travis CI setup.
Bump the minimum PHP version to 5.3.2 because of Composer's minimum requirements.
Add a php-build script that basically runs composer 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:

Make sure you're not connected to WP.com, but Jetpack is activated.
Go to /wp-admin/plugins.php
Make sure the logo is displayed properly.
Run the tests and make sure they pass: yarn docker:phpunit --testsuite packages

Proposed changelog entry for your changes:

Introduced a new Jetpack Logo package.

Notes

Architecture of how tests are structured is likely going to change soon. While they're still in the global directory, we're going to move them out to the packages and make them run both independently and together as a whole. This should be good for now though.
We'll have to build a fresh composer.lock after releasing the logo as a package.

jetpackbot · 2019-05-15T12:52:00Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: June 4, 2019.
Scheduled code freeze: May 28, 2019

Generated by 🚫 dangerJS against 500e664

jeherve · 2019-05-15T12:55:25Z

.phpcs.xml.dist

 	<rule ref="PHPCompatibilityWP"/>
-	<rule ref="WordPress-Core" />
+	<rule ref="WordPress-Core">
+		<!-- Can we disable this just for `packages/*`? -->


I think it may be possible:

 <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

Lovely, thanks, @jeherve! Done in 550a64a

gravityrail · 2019-05-15T18:35:32Z

@tyxla wrote:

Bump to PHP 5.6, which we'll be able to do without remorse when the new WP major version is released.

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

gititon · 2019-05-15T21:57:40Z

https://github.com/Automattic/jetpack/pull/new/try/composer-jetpack-logo-package-singleton-interfaces

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.

tyxla · 2019-05-20T12:25:04Z

@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/.gitignore

aldavigdis · 2019-05-21T13:22:21Z

packages/logo/src/Logo.php

+		return sprintf(
+			'<img src="%s" class="jetpack-logo" alt="%s" />',
+			esc_url( $this->url ),
+			esc_attr__(


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.

aldavigdis · 2019-05-21T13:22:41Z

packages/logo/src/Logo.php

+			'<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',


Suggested change

'Jetpack is a free plugin that utilizes powerful WordPress.com servers to enhance your site and simplify managing it',

'Jetpack',

Is there a different attribute we could use to put the old string as extra information?

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?

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.

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.

Ah, I've also removed the localization function there, because we no longer need it - we don't translate Jetpack AFAIK.

I'd still wrap it with a __ function, just in case.

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.

packages/logo/src/Logo.php

tests/php/packages/logo/test_Logo.php

scottsweb · 2019-05-22T12:06:31Z

Design changes: Changed the original alt attribute to just Jetpack.

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.

jeherve

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

jetpack.php

composer.json

jetpack.php

See #12379 (comment) for more discussion about this.

kraftbj · 2019-05-22T21:08:58Z

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>

tyxla added [Status] Needs Review This PR is ready for review. Task DO NOT MERGE don't merge it! [Focus] Jetpack DNA labels May 15, 2019

tyxla requested review from a team May 15, 2019 12:48

tyxla self-assigned this May 15, 2019

jeherve reviewed May 15, 2019

View reviewed changes

tyxla added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels May 15, 2019

tyxla mentioned this pull request May 17, 2019

Try/composer jetpack logo dependecy injection #12400

Closed

tyxla force-pushed the try/composer-jetpack-logo-package branch from 7660f4d to f9e79a9 Compare May 20, 2019 12:15

simison reviewed May 20, 2019

View reviewed changes

packages/.gitignore Show resolved Hide resolved

aldavigdis reviewed May 21, 2019

View reviewed changes

gravityrail mentioned this pull request May 21, 2019

Add logo as dependent package #12363

Closed

tyxla force-pushed the try/composer-jetpack-logo-package branch from e700669 to b64ede1 Compare May 22, 2019 09:13

tyxla added this to the 7.4 milestone May 22, 2019

tyxla added [Status] Needs Review This PR is ready for review. [Status] Needs Design Review Design has been added. Needs a review! and removed DO NOT MERGE don't merge it! [Status] In Progress labels May 22, 2019

tyxla requested review from a team May 22, 2019 11:34

scottsweb removed the [Status] Needs Design Review Design has been added. Needs a review! label May 22, 2019

tyxla and others added 13 commits May 22, 2019 15:14

Remove any static methods

64c097c

Fix usage in Jetpack_Connection_Banner

d0c1e73

Fix tests

ec13598

Add some newlines in tests

5dc7f28

Update package README

cbd8614

Update composer.lock and composer.json

f6ac0c5

Shorten alt attribute

b4342b6

Bump the minimum PHP version because of Composer

1226584

Move autoloader below the PHP version check

9ebdb42

Add dat dot.

907e8f8

Correct comment

9bf50d9

Add a build-php step

662502b

only ignore dev deps

0a77889

jeherve reviewed May 22, 2019

View reviewed changes

jetpack.php Outdated Show resolved Hide resolved

composer.json Outdated Show resolved Hide resolved

kraftbj force-pushed the try/composer-jetpack-logo-package branch from 8431c25 to 0a77889 Compare May 22, 2019 20:31

jeherve reviewed May 22, 2019

View reviewed changes

jetpack.php Outdated Show resolved Hide resolved

Fail gracefully if the autoloader is not available yet.

500e664

See #12379 (comment) for more discussion about this.

kraftbj approved these changes May 22, 2019

View reviewed changes

kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 22, 2019

kraftbj merged commit 40d0573 into master May 22, 2019

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 22, 2019

kraftbj deleted the try/composer-jetpack-logo-package branch May 22, 2019 21:09

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels May 23, 2019

jeherve added a commit that referenced this pull request May 23, 2019

changelog: add #12379

196b9a3

	'Jetpack is a free plugin that utilizes powerful WordPress.com servers to enhance your site and simplify managing it',
	'Jetpack',

Conversation

tyxla commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

Notes

Uh oh!

jetpackbot commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gravityrail commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gititon commented May 15, 2019

Uh oh!

tyxla commented May 20, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lezama May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

scottsweb commented May 22, 2019

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kraftbj commented May 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

tyxla commented May 15, 2019 •

edited

Loading

jetpackbot commented May 15, 2019 •

edited

Loading

gravityrail commented May 15, 2019 •

edited

Loading

lezama May 21, 2019 •

edited

Loading