Skip to content
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

Remove inline style colors atributes from paragraph. #3722

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion blocks/api/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
export { createBlock, switchToBlockType, createReusableBlock } from './factory';
export { default as parse, getBlockAttributes } from './parser';
export { default as rawHandler } from './raw-handling';
export { default as serialize, getBlockDefaultClassname, getBlockContent } from './serializer';
export { default as serialize, getBlockDefaultClassname, getBlockRandomAnchor, getBlockContent } from './serializer';
export { isValidBlock } from './validation';
export { getCategories } from './categories';
export {
Expand Down
36 changes: 33 additions & 3 deletions blocks/api/serializer.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { isEmpty, reduce, isObject, castArray, compact, startsWith } from 'lodash';
import { isEmpty, reduce, isObject, castArray, compact, startsWith, random } from 'lodash';
import { html as beautifyHtml } from 'js-beautify';

/**
Expand All @@ -15,15 +15,45 @@ import { applyFilters } from '@wordpress/hooks';
*/
import { getBlockType, getUnknownTypeHandlerName } from './registration';

/**
* Returns the block's name with common prefixes ('core/', 'core-') dropped
*
* @param {String} blockName The block name
* @return {string} Friendly name
*/
function friendlyBlockName( blockName ) {
return blockName.replace( /\//, '-' ).replace( /^core-/, '' );
}

/**
* Returns the block's default classname from its name
*
* @param {String} blockName The block name
* @return {string} The block's default class
*/
export function getBlockDefaultClassname( blockName ) {
// Drop common prefixes: 'core/' or 'core-' (in 'core-embed/')
return 'wp-block-' + blockName.replace( /\//, '-' ).replace( /^core-/, '' );
return `wp-block-${ friendlyBlockName( blockName ) }`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure it's worth concerning about, but there's a decent amount of redundancy between the class name and style applied to a block with custom styles, i.e.:

<p id="wp-block-paragraph-z60dssg" class="wp-block-paragraph">
</p>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the naming was redundant, I changed the id's so now they are in format 'paragraph-....'.

}

/**
* Returns a random base 36 string, 36^7 possible strings, maximum seven chars
* According to the birthday paradox the probability of repetition is given by 1 - e^(-k*(k-1)/(2*36^7)) where k is the number of strings generated
* If used to generate ids it's relatively safe as the expected number of ids we have to generate before finding the first collision is approximately 350848
*
* @return {string} Random string
*/
function randomBase36String() {
return random( 0, Math.pow( 36, 7 ), false ).toString( 36 );
}

/**
* Returns random anchor for the block
*
* @param {String} blockName The block name
* @return {string} The block's default class
*/
export function getBlockRandomAnchor( blockName ) {
return `${ friendlyBlockName( blockName ) }-${ randomBase36String() }`;
}

/**
Expand Down
3 changes: 0 additions & 3 deletions blocks/hooks/anchor.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ export function addAttribute( settings ) {
settings.attributes = assign( settings.attributes, {
anchor: {
type: 'string',
source: 'attribute',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this attributes definition modified?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change allows the server to have access to anchor attribute which is required to generate the styles in the server.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change allows the server to have access to anchor attribute which is required to generate the styles in the server.

This will also mean existing blocks with anchors will lose the attribute value after this change (since it was not encoded in the comment delimiters).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this was mentioned in #3722 (comment)

attribute: 'id',
selector: '*',
},
} );
}
Expand Down
56 changes: 56 additions & 0 deletions blocks/hooks/custom-styles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import { getBlockRandomAnchor, hasBlockSupport } from '../api';

const CUSTOM_STYLE_ATTRIBUTES = [ 'backgroundColor', 'textColor' ];

const hasCustomStyleAttributes = ( attributes ) => (
CUSTOM_STYLE_ATTRIBUTES.some( attr => attributes[ attr ] )
);

function withCustomStylesAnchor( BlockEdit ) {
return class CustomStylesAnchor extends Component {
componentWillReceiveProps( nextProps ) {
if ( hasBlockSupport( nextProps.name, 'anchor' ) &&
hasBlockSupport( nextProps.name, 'customStyles' ) &&
! nextProps.attributes.anchor &&
hasCustomStyleAttributes( nextProps.attributes )
) {
nextProps.setAttributes( {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain what setAttributes is doing here? In general props should be readonly, but it's hard to find out if this call updates props or not. Given that we check whether nextProps.attributes.anchor is empty, I would assume this code updates this attribute. If I correctly assumed, we might want to introduce internal state here, and combine props and state when passing down to the <BlockEdit { ...this.props } /> in render method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gziolo, what we are doing here is verifying if no anchor was set yet ( by the user or automatically). If that's the case we check if attributes needing custom styles were used and if yes we set an automatically generated anchor attribute. I think it's not enough to pass a prop we really want to save the attribute for it to be serialized.

The general question here is what is our extensibility solution to allow to set an attribute depending on other. E.g: If I want to have a plugin that sets red text color attribute if the background is blue what would be the best way to do it? Right now it looks like using our hooks we can achieve this result using this solution, but maybe there is a simpler way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, setAttributes is some magic functions passed down to edit which does all magic. It must be good 👍

anchor: getBlockRandomAnchor( nextProps.name ),
} );
}
}

render() {
return (
<BlockEdit { ...this.props } />
);
}
};
}

export function addCustomStylesClass( extraProps, blockType, attributes ) {
if ( hasBlockSupport( blockType.name, 'customStyles' ) && hasCustomStyleAttributes( attributes ) ) {
extraProps.className = classnames( extraProps.className, 'has-custom-styles' );
}

return extraProps;
}

export default function customAnchorStyles() {
addFilter( 'blocks.BlockEdit', 'core/custom-styles/inspector-control', withCustomStylesAnchor );
addFilter( 'blocks.getSaveContent.extraProps', 'core/custom-styles/save-props', addCustomStylesClass );
}
2 changes: 2 additions & 0 deletions blocks/hooks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
*/
import anchor from './anchor';
import customClassName from './custom-class-name';
import customStyles from './custom-styles';
import generatedClassName from './generated-class-name';
import matchers from './matchers';

anchor();
customClassName();
customStyles();
generatedClassName();
matchers();
6 changes: 3 additions & 3 deletions blocks/library/paragraph/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ registerBlockType( 'core/paragraph', {

supports: {
className: false,
anchor: true,
customStyles: true,
},

attributes: {
Expand Down Expand Up @@ -267,15 +269,13 @@ registerBlockType( 'core/paragraph', {
edit: ParagraphBlock,

save( { attributes } ) {
const { width, align, content, dropCap, backgroundColor, textColor, fontSize } = attributes;
const { width, align, content, dropCap, backgroundColor, fontSize } = attributes;
const className = classnames( {
[ `align${ width }` ]: width,
'has-background': backgroundColor,
'has-drop-cap': dropCap,
} );
const styles = {
backgroundColor: backgroundColor,
color: textColor,
fontSize: fontSize,
textAlign: align,
};
Expand Down
35 changes: 34 additions & 1 deletion lib/blocks.php
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,39 @@ function gutenberg_render_block( $block ) {
return '';
}

/**
* Given an array of parsed blocks, returns the css styles they require.
*
* @since 1.8.1
*
* @param array $blocks Parsed block array.
*
* @return string CSS style block or empty string
*/
function do_blocks_styles( $blocks ) {
$map_attrs_styles = array(
'textColor' => 'color',
'backgroundColor' => 'background-color',
);

$styles = '';
foreach ( $blocks as $block ) {
$attributes = is_array( $block['attrs'] ) ? $block['attrs'] : array();
if ( isset( $attributes['anchor'] ) ) {
$rules = '';
foreach ( $map_attrs_styles as $attr_key => $css_property ) {
if ( isset( $attributes[ $attr_key ] ) ) {
$rules .= "\n\t" . esc_attr( sprintf( '%1$s: %2$s;', $css_property, $attributes[ $attr_key ] ) );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to do the whitespace prettifying here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this whitespace prettifying when we check the source code the styles are messy and all on the same line but they work. I'm not certain what we normally do in this cases.

}
}
if ( '' !== $rules ) {
$styles .= sprintf( "\n.has-custom-styles#%1\$s { %2\$s\n}", esc_attr( $attributes['anchor'] ), $rules );
}
}
}
return '' !== $styles ? sprintf( "<style type\"text/css\">%1\$s\n</style>", $styles ) : '';
}

/**
* Parses dynamic blocks out of `post_content` and re-renders them.
*
Expand All @@ -182,7 +215,7 @@ function gutenberg_render_block( $block ) {
function do_blocks( $content ) {
$blocks = gutenberg_parse_blocks( $content );

$content_after_blocks = '';
$content_after_blocks = do_blocks_styles( $blocks );
foreach ( $blocks as $block ) {
$content_after_blocks .= gutenberg_render_block( $block );
}
Expand Down