Share the block style selector code #14383

scruffian · 2020-01-17T13:49:46Z

Fixes #14466

Changes proposed in this Pull Request:

OpenTable and Calendly both have a block style selector which does essentially the same thing. Let's extract that to a new shared component.

Testing instructions:

Add a Calendly block and check that you can still change the styles in the sidebar
Add an OpenTable block and check that you can still change the styles in the sidebar

Proposed changelog entry for your changes:

no changelog

matticbot · 2020-01-17T13:49:53Z

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

jetpackbot · 2020-01-17T13:51: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: February 11, 2020.
Scheduled code freeze: February 4, 2020

Generated by 🚫 dangerJS against 65f08b7

extensions/shared/components/block-styles-selector/style.scss

matticbot · 2020-01-20T16:31:44Z

scruffian, Your synced wpcom patch D37842-code has been updated.

simison · 2020-01-21T09:15:30Z

I'm sure this has been discussed somewhere and I'm not questioning this, but I'm curious why not just use Gutenberg's built-in styles feature? It also gives similar looking style picker in the sidebar.

scruffian · 2020-01-21T13:58:52Z

Gutenberg's built in style switcher only deals with changing the class name for the block, not for changing attributes, which is how this works.

creativecoder · 2020-01-21T17:10:52Z

@scruffian

We're using a similar approach in the Eventbrite block, but using an image rather than a block preview for the "in page" embed option, since that option takes a while to load (during which the preview is blank). https://github.com/Automattic/jetpack/pull/14075/files#diff-67c4a00b3bd3c01b617670a5fd26e417R148

What do you think about making this more generic by using a prop for rendering the preview?

scruffian · 2020-01-21T18:22:42Z

We could make it more generic by passing in the preview mechanism as a function. However I'd rather keep this component as close to the core component as possible, with the hope of eventually getting it merged in there. We could look into how to integrate with Eventbrite once this (or that) PR merges...

creativecoder · 2020-01-21T20:08:14Z

However I'd rather keep this component as close to the core component as possible, with the hope of eventually getting it merged in there.

I'd make the same argument in the Core context! The pattern is more broadly usable if it doesn't lock you into a very specific way of rendering a block preview, like it does now. But do what you think is best--what I have now for the Eventbrite block works just fine.

simison · 2020-01-22T10:34:09Z

Gutenberg's built in style switcher only deals with changing the class name for the block, not for changing attributes, which is how this works.

Oh yea, that's annoying limitation indeed. We used style picker in Tiled gallery block but needed the style information in JS as well. There's a helper for that in core and in Jetpack:

jetpack/extensions/blocks/tiled-gallery/edit.js

Line 223 in aa3ad69

const layoutStyle = getActiveStyleName( LAYOUT_STYLES, attributes.className );

jetpack/extensions/shared/block-styles.js

Lines 7 to 36 in aa3ad69

    
           /** 
        
            * Returns the active style from the given className. 
        
            * 
        
            * From @link https://github.com/WordPress/gutenberg/blob/ddac4f3cf8fd311169c7e125411343a437bdbb5a/packages/editor/src/components/block-styles/index.js#L20-L42 
        
            * 
        
            * @param {Array} styles Block style variations. 
        
            * @param {string} className  Class name 
        
            * 
        
            * @return {Object?} The active style. 
        
            */ 
        
           function getActiveStyle( styles, className ) { 
        
           	for ( const style of new TokenList( className ).values() ) { 
        
           		if ( style.indexOf( 'is-style-' ) === -1 ) { 
        
           			continue; 
        
           		} 
        
           		const potentialStyleName = style.substring( 9 ); 
        
           		const activeStyle = find( styles, { name: potentialStyleName } ); 
        
           		if ( activeStyle ) { 
        
           			return activeStyle; 
        
           		} 
        
           	} 
        
           	return find( styles, 'isDefault' ); 
        
           } 
        
           export function getActiveStyleName( styles, className ) { 
        
           	const style = getActiveStyle( styles, className ); 
        
           	return style ? style.name : null; 
        
           }

By defining styles semantically in the block, you give Gutenberg UI a chance to decide best how to surface that; it might change over time or be surfaced differently in various editor instances (think mobile app version, mobile web version, widgets screen, full site editing mode, writing mode, and generally things we haven't seen yet.)

scruffian · 2020-01-23T11:11:43Z

@simison I agree, this needs more thought. I think this PR is still valuable in removing the duplicate code, but we should think more about how we interact with block styles - in a different PR!

simison · 2020-01-23T11:17:37Z

I think this PR is still valuable in removing the duplicate code, but we should think more about how we interact with block styles - in a different PR!

Absolutely! 👍 :-) Just wanted to highlight other mechanisms in case.

jeherve

This will need a rebase now, and tests are failing because of the React import.

jeherve · 2020-01-27T11:41:07Z

extensions/shared/components/block-styles-selector/index.js

+ * External Dependencies
+ */
+import classnames from 'classnames';
+import { memo } from 'React';


This is breaking things:

Module not found: Error: Can't resolve 'React' in '/home/travis/build/Automattic/jetpack/extensions/shared/components/block-styles-selector'

Can we do without all of React here? Can we get it from @wordpress/element instead?

There is a useMemo HOC in @wordpress/element, but when I use this I get an error from React. Looking into it some more...

Ok I added a commit to take care of this too.

We might be able to change this to useMemo but we need useSelect which (as you've seen) also isn't available. It's a shame, but treating it like we have for the changes to BlockPreview makes sense to me.

matticbot · 2020-01-27T12:40:40Z

scruffian, Your synced wpcom patch D37842-code has been updated.

scruffian · 2020-01-27T15:20:58Z

I rebased this and its working fine. I added a commit to just hide the preview if the version of WordPress doesn't support it:

This seems like the simplest solution for now.

scruffian · 2020-01-27T15:54:29Z

I added a fix for #14466 too.

pablinos · 2020-01-27T17:48:03Z

Looks good, and works as expected. It's a shame about the hooks, but never mind.

extensions/shared/components/block-styles-selector/index.js

jeherve

This seems to work well on my end. I'll merge this now, and take care of the matching wpcom reference since it introduces a new file.

Should we now update the Eventbrite block to use this too?

* share the block style selector code * Memoized the style previews to prevent them from rerendering * remove unused CSS * Remove unused class * Dont show block previews unless they are supported * use the internal memo if it exists * don't try to render the style selector if there's no valid block Co-authored-by: Paul Bunkham <[email protected]>

jeherve · 2020-01-30T12:05:18Z

Cherry-picked to branch-8.2 in b878a81

jeherve · 2020-01-30T13:17:31Z

r202313-wpcom

jeherve · 2020-01-30T13:54:53Z

Should we now update the Eventbrite block to use this too?

Logged in #14527

scruffian added [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack [Block] OpenTable [Block] Calendly labels Jan 17, 2020

scruffian requested a review from a team January 17, 2020 13:49

scruffian self-assigned this Jan 17, 2020

matticbot added the Touches WP.com Files label Jan 17, 2020

scruffian commented Jan 17, 2020

View reviewed changes

extensions/shared/components/block-styles-selector/style.scss Outdated Show resolved Hide resolved

scruffian added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 20, 2020

jeherve requested changes Jan 27, 2020

View reviewed changes

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 27, 2020

jeherve added this to the 8.3 milestone Jan 27, 2020

scruffian and others added 4 commits January 27, 2020 12:40

share the block style selector code

2828070

Memoized the style previews to prevent them from rerendering

f382a69

remove unused CSS

23f84f1

Remove unused class

98a0b3e

scruffian force-pushed the update/share-block-style-selector branch from 4ee07e7 to 98a0b3e Compare January 27, 2020 12:40

Dont show block previews unless they are supported

1cb0842

use the internal memo if it exists

6ef28c3

scruffian added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 27, 2020

don't try to render the style selector if there's no valid block

65f08b7

jeherve modified the milestones: 8.3, 8.2 Jan 29, 2020

jeherve added the [Status] Needs Cherry-Pick label Jan 29, 2020

jeherve requested changes Jan 29, 2020

View reviewed changes

extensions/shared/components/block-styles-selector/index.js Show resolved Hide resolved

jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 29, 2020

jeherve approved these changes Jan 30, 2020

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 30, 2020

jeherve merged commit 36c4883 into master Jan 30, 2020

jeherve deleted the update/share-block-style-selector branch January 30, 2020 12:04

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 30, 2020

jeherve removed [Status] Needs Changelog [Status] Needs Cherry-Pick labels Jan 30, 2020

jeherve mentioned this pull request Jan 30, 2020

Eventbrite: switch to shared block styles #14527

Closed

Aurorum mentioned this pull request Oct 21, 2023

Gutenberg: Add Goodreads Block #33395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share the block style selector code #14383

Share the block style selector code #14383

scruffian commented Jan 17, 2020 •

edited by jeherve

Loading

matticbot commented Jan 17, 2020

jetpackbot commented Jan 17, 2020 •

edited

Loading

matticbot commented Jan 20, 2020

simison commented Jan 21, 2020

scruffian commented Jan 21, 2020

creativecoder commented Jan 21, 2020

scruffian commented Jan 21, 2020

creativecoder commented Jan 21, 2020

simison commented Jan 22, 2020

scruffian commented Jan 23, 2020

simison commented Jan 23, 2020

jeherve left a comment

jeherve Jan 27, 2020

scruffian Jan 27, 2020

scruffian Jan 27, 2020

pablinos Jan 27, 2020

matticbot commented Jan 27, 2020

scruffian commented Jan 27, 2020

scruffian commented Jan 27, 2020

pablinos commented Jan 27, 2020

jeherve left a comment

jeherve commented Jan 30, 2020

jeherve commented Jan 30, 2020

jeherve commented Jan 30, 2020

Share the block style selector code #14383

Share the block style selector code #14383

Conversation

scruffian commented Jan 17, 2020 • edited by jeherve Loading

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

matticbot commented Jan 17, 2020

jetpackbot commented Jan 17, 2020 • edited Loading

matticbot commented Jan 20, 2020

simison commented Jan 21, 2020

scruffian commented Jan 21, 2020

creativecoder commented Jan 21, 2020

scruffian commented Jan 21, 2020

creativecoder commented Jan 21, 2020

simison commented Jan 22, 2020

scruffian commented Jan 23, 2020

simison commented Jan 23, 2020

jeherve left a comment

Choose a reason for hiding this comment

jeherve Jan 27, 2020

Choose a reason for hiding this comment

scruffian Jan 27, 2020

Choose a reason for hiding this comment

scruffian Jan 27, 2020

Choose a reason for hiding this comment

pablinos Jan 27, 2020

Choose a reason for hiding this comment

matticbot commented Jan 27, 2020

scruffian commented Jan 27, 2020

scruffian commented Jan 27, 2020

pablinos commented Jan 27, 2020

jeherve left a comment

Choose a reason for hiding this comment

jeherve commented Jan 30, 2020

jeherve commented Jan 30, 2020

jeherve commented Jan 30, 2020

scruffian commented Jan 17, 2020 •

edited by jeherve

Loading

jetpackbot commented Jan 17, 2020 •

edited

Loading