Gutenblocks: add new utility to get all classes for a given block #13302

jeherve · 2019-08-22T15:35:47Z

Fixes #13257

Changes proposed in this Pull Request:

We previously fetched different classes in each block rendering function, from multiple places (the block's name, the block's alignment settings, the block's extra CSS classes that can be defined in the sidebar, and sometimes our own extra classes). This PR centralizes things into a common function that can be used by other blocks in the future.
This PR also changes the default alignment for the Mailchimp and the GIF block. Until now we defaulted to a center alignment, which may not be the expected default. Thus this fixes Mailchimp block: Hardcoded alignment setting #13257.

Testing instructions:

You can test this PR by inserting a GIF, Recurring Payments, Mailchimp, and / or Repeat Visitor block in the block editor.
After inserting the block, you can preview your post.
You can check the block's markup and make sure the overall div container displays at least the main block class, wp-block-jetpack-blockname.
You should not see an aligncenter class name by default.
Back in the block editor, try making some changes to your block:
- Add a custom class using the CSS class input field at the bottom of the editor sidebar when the block is selected.
- Define an alignment for your block when it's offered.
You will want to make sure those extra fields appear in the block markup in the preview.

Proposed changelog entry for your changes:

Blocks: add new utility to get all classes for a given block.

matticbot · 2019-08-22T15:35:54Z

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D31844-code before merging this PR. Thank you!

jetpackbot · 2019-08-22T15:37:44Z

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: September 3, 2019.
Scheduled code freeze: August 27, 2019

Generated by 🚫 dangerJS against 6694fae

simison · 2019-08-23T06:03:49Z

@jeffersonrabb I added you as a reviewer here because I remember you were particularly interested to see this happen unless I remember wrong. but no pressure/feel free to ignore. ;-)

extensions/blocks/repeat-visitor/repeat-visitor.php

class.jetpack-gutenberg.php

ockham · 2019-08-23T06:21:54Z

class.jetpack-gutenberg.php

+			&& in_array( $attr['align'], array( 'left', 'center', 'right', 'wide', 'full' ), true )
+		) {
+			array_push( $classes, 'align' . $attr['align'] );
+		}


Do we even have to take care of alignment? I thought we could pretty much rely on Gutenberg's supports feature for alignment... 🤔 I'll try to dig up a PR I vaguely recall where I was doing something like that...

I think some of our blocks are little old in that we handled it manually. Gutenberg's support might be purely frontend side and backend side has to be handled manually like this?

For reference, this is the commit I was thinking of: Automattic/wp-calypso@d675ab0

I'll see if that kind of change can be applied here...

Ah, nevermind, that doesn't really apply to dynamic blocks/the server side/PHP.

class.jetpack-gutenberg.php

See #13302 (comment)

matticbot · 2019-08-23T07:03:53Z

jeherve, Your synced wpcom patch D31844-code has been updated.

It's already specified earlier anyway. See https://github.com/Automattic/jetpack/pull/13302/files#r316986528 Co-Authored-By: Bernie Reiter <ockham@raz.or.at>

matticbot · 2019-08-23T07:05:14Z

jeherve, Your synced wpcom patch D31844-code has been updated.

simison

Tested gif, recurring payments, repeat visitor, and Mailchimp blocks. Tested with gif's align options and with custom CSS classes for each.

Super happy to see maintenance like this. Well done!

empty does check if it is set anyway. See #13302 (comment)

matticbot · 2019-08-23T07:07:20Z

jeherve, Your synced wpcom patch D31844-code has been updated.

ockham · 2019-08-23T08:37:06Z

modules/memberships/class-jetpack-memberships.php

+				'wp-block-button__link',
+				'components-button',
+				'is-primary',
+				'is-button',


Not entirely sure classes like this belong into a top-level block element, that's pre-existing. (It might raise the question if the new util is really needed, or more of a symptom of suboptimal architectural choices, but oh well.)

ockham

LGTM 👍

It's almost curious that core doesn't provide a helper like this for dynamic blocks (and might be a worthwhile addition).

simison · 2019-08-23T13:39:42Z

class.jetpack-gutenberg.php

+
+		// Basic block name class.
+		$classes = array(
+			'wp-block-jetpack-' . $slug,


Just noting that (I think) setting className:false in block's supports attributes would normally disable the primary class name as well but this always enforces it.

I don't think we'll ever have any such blocks in Jetpack so it doesn't really matter, but wanted to note in case anyone looks up this PR later on for inspiration to implement this also in core Gutenberg.

jeherve · 2019-08-23T13:45:41Z

Bringing Design for a second opinion here about the default alignment for blocks.

See #13257 for a point of view on this, and p1566567543002100-slack-jetpack-plugin for another.

jeffersonrabb · 2019-08-23T13:54:50Z

For reference here's how we're doing the same in Newspack, although this PR has a bit of sophistication we'll want to add to Newspack's:

https://github.com/Automattic/newspack-blocks/blob/master/newspack-blocks.php#L147-L164

simison · 2019-08-26T08:05:14Z

Bringing Design for a second opinion here about the default alignment for blocks.

I think it's ok to address separate since this PR doesn't really change that behavior, right?

jeherve · 2019-08-26T08:18:36Z

this PR doesn't really change that behavior, right?

It does, for the Mailchimp and the Gif blocks. See #13257 for the previous behaviour.

jeffersonrabb

LGTM. The center alignment in GIF and Mailchimp was just a reasonable default, I don't think we'll miss it.

* 7.7 changelog: initial set of changes * Changelog: add #13154 * Changelog: add #13134 * Changelog: add #12699 and many others * Changelog: add #13127 * Changelog: add #13167 * Changelog: add #13225 * Changelog: add #13179 * Changelog: add #13173 * Changelog: add #13232 * Changelog: add #13137 * Changelog: add #13172 * Changelog: add #13182 * Changelog: add #13200 * Changelog: add #13244 * Changelog: add #13267 * Changelog: add #13204 * changelog: add #13205 * Changelog: add #13183 * Changelog: add #13278 * Changelog: add #13162 * Changelog: add #13268 * Changelog: add #13286 * Changelog: add #13273 * Changelog: add #12474 * Changelog: add #13085 * Changelog: add #13266 * Changelog: add #13306 * Changelog: add #13311 * Changelog: add #13302 * Changelog: add #13196 * Changelog: add #12733 * Changelog: add #13261 * Changelog: add #13322 * Changelog: add #13333 * Changelog: add #13335

jeherve added 2 commits August 22, 2019 17:26

Gutenberg: add new utility to get all CSS classes for a block.

e5a38a0

Use new block_classes in all blocks that rely on classes.

10708f1

jeherve added this to the 7.7 milestone Aug 22, 2019

jeherve requested review from a team August 22, 2019 15:35

jeherve self-assigned this Aug 22, 2019

matticbot added the Touches WP.com Files label Aug 22, 2019

jeherve requested a review from iamtakashi August 22, 2019 15:37

jeherve mentioned this pull request Aug 22, 2019

Mailchimp block: Hardcoded alignment setting #13257

Closed

simison requested a review from jeffersonrabb August 23, 2019 06:03

simison reviewed Aug 23, 2019

View reviewed changes

extensions/blocks/repeat-visitor/repeat-visitor.php Outdated Show resolved Hide resolved

ockham reviewed Aug 23, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

ockham reviewed Aug 23, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

ockham reviewed Aug 23, 2019

View reviewed changes

simison reviewed Aug 23, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

Escape class output

f8914ba

See #13302 (comment)

Omit the data type from the description

b3546c7

It's already specified earlier anyway. See https://github.com/Automattic/jetpack/pull/13302/files#r316986528 Co-Authored-By: Bernie Reiter <ockham@raz.or.at>

simison previously approved these changes Aug 23, 2019

View reviewed changes

Check if empty instead of isset

6694fae

empty does check if it is set anyway. See #13302 (comment)

jeherve dismissed simison’s stale review via 6694fae August 23, 2019 07:07

ockham reviewed Aug 23, 2019

View reviewed changes

ockham approved these changes Aug 23, 2019

View reviewed changes

simison approved these changes Aug 23, 2019

View reviewed changes

simison reviewed Aug 23, 2019

View reviewed changes

jeherve added the [Status] Needs Design Review Design has been added. Needs a review! label Aug 23, 2019

artpi mentioned this pull request Aug 23, 2019

Recurring Payments: change alignment #13310

Merged

jeffersonrabb approved these changes Aug 26, 2019

View reviewed changes

oskosk approved these changes Aug 26, 2019

View reviewed changes

oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 26, 2019

jeherve merged commit ef39055 into master Aug 26, 2019

jeherve deleted the add/block-class-utility branch August 26, 2019 11:29

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

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Aug 26, 2019

jeherve added a commit that referenced this pull request Aug 26, 2019

Changelog: add #13302

64997bc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gutenblocks: add new utility to get all classes for a given block #13302

Gutenblocks: add new utility to get all classes for a given block #13302

jeherve commented Aug 22, 2019

matticbot commented Aug 22, 2019

jetpackbot commented Aug 22, 2019 •

edited

Loading

simison commented Aug 23, 2019

ockham Aug 23, 2019

simison Aug 23, 2019

ockham Aug 23, 2019

ockham Aug 23, 2019

matticbot commented Aug 23, 2019

matticbot commented Aug 23, 2019

simison left a comment •

edited

Loading

matticbot commented Aug 23, 2019

ockham Aug 23, 2019

ockham left a comment

simison Aug 23, 2019

jeherve commented Aug 23, 2019

jeffersonrabb commented Aug 23, 2019

simison commented Aug 26, 2019

jeherve commented Aug 26, 2019

jeffersonrabb left a comment

Gutenblocks: add new utility to get all classes for a given block #13302

Gutenblocks: add new utility to get all classes for a given block #13302

Conversation

jeherve commented Aug 22, 2019

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Aug 22, 2019

jetpackbot commented Aug 22, 2019 • edited Loading

simison commented Aug 23, 2019

ockham Aug 23, 2019

Choose a reason for hiding this comment

simison Aug 23, 2019

Choose a reason for hiding this comment

ockham Aug 23, 2019

Choose a reason for hiding this comment

ockham Aug 23, 2019

Choose a reason for hiding this comment

matticbot commented Aug 23, 2019

matticbot commented Aug 23, 2019

simison left a comment • edited Loading

Choose a reason for hiding this comment

matticbot commented Aug 23, 2019

ockham Aug 23, 2019

Choose a reason for hiding this comment

ockham left a comment

Choose a reason for hiding this comment

simison Aug 23, 2019

Choose a reason for hiding this comment

jeherve commented Aug 23, 2019

jeffersonrabb commented Aug 23, 2019

simison commented Aug 26, 2019

jeherve commented Aug 26, 2019

jeffersonrabb left a comment

Choose a reason for hiding this comment

jetpackbot commented Aug 22, 2019 •

edited

Loading

simison left a comment •

edited

Loading