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.

Sign up for GitHub

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

Jump to bottom

Update Post Author block to use __experimentalColor and __experimentalLineHeight #23044

Merged
merged 11 commits into from
Jul 20, 2020
Merged

Update Post Author block to use __experimentalColor and __experimentalLineHeight #23044

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
55c199b
add the new support flag
Addison-Stavlo Jun 9, 2020
16cb05f
remove the old
Addison-Stavlo Jun 9, 2020
319c117
remove unnecessary attributes
Addison-Stavlo Jun 9, 2020
ddd51f6
updated colors in php
Addison-Stavlo Jun 9, 2020
d17d371
add link color support
Addison-Stavlo Jun 9, 2020
b84f453
update background classname
Addison-Stavlo Jun 11, 2020
d4bcbfd
update from master
Addison-Stavlo Jul 14, 2020
2a33335
gutting the unnecessary
Addison-Stavlo Jul 14, 2020
fdbdaef
add style rules to add the set link color
Addison-Stavlo Jul 16, 2020
c3c40a5
Merge branch 'master' into update/post-author-block-experimental-colors
Addison-Stavlo Jul 16, 2020
ab224b2
remove important style
Addison-Stavlo Jul 20, 2020
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
18 changes: 5 additions & 13 deletions packages/block-library/src/post-author/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,6 @@
},
"byline": {
"type": "string"
},
"backgroundColor": {
"type": "string"
},
"textColor": {
"type": "string"
},
"customBackgroundColor": {
"type": "string"
},
"customTextColor": {
"type": "string"
}
},
"context": [
Expand All @@ -38,6 +26,10 @@
],
"supports": {
"html": false,
"__experimentalFontSize": true
"__experimentalFontSize": true,
"__experimentalColor": {
"gradients": true,
"linkColor": true
}
}
}
138 changes: 37 additions & 101 deletions packages/block-library/src/post-author/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,23 @@
/**
* External dependencies
*/
import { forEach, groupBy } from 'lodash';
import { forEach } from 'lodash';
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { useRef, useMemo } from '@wordpress/element';
import { useMemo } from '@wordpress/element';
import {
AlignmentToolbar,
BlockControls,
InspectorControls,
RichText,
__experimentalUseColors,
BlockColorsStyleSelector,
} from '@wordpress/block-editor';
import { PanelBody, SelectControl, ToggleControl } from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { __ } from '@wordpress/i18n';

const DEFAULT_CONTRAST_CHECK_FONT_SIZE = 12;

function PostAuthorEdit( { isSelected, context, attributes, setAttributes } ) {
const { postType, postId } = context;

Expand All @@ -47,53 +43,6 @@ function PostAuthorEdit( { isSelected, context, attributes, setAttributes } ) {

const { editEntityRecord } = useDispatch( 'core' );

// Need font size in number form for named presets to be used in contrastCheckers.
const { fontSizes } = useSelect( ( select ) =>
select( 'core/block-editor' ).getSettings()
);
const fontSizeIndex = useMemo( () => groupBy( fontSizes, 'slug' ), [
fontSizes,
] );
const contrastCheckFontSize = useMemo(
() =>
// Custom size if set.
attributes.style?.typography?.fontSize ||
// Size of preset/named value if set.
fontSizeIndex[ attributes.fontSize ]?.[ 0 ].size ||
DEFAULT_CONTRAST_CHECK_FONT_SIZE,
[
attributes.style?.typography?.fontSize,
attributes.fontSize,
fontSizeIndex,
]
);
Comment on lines -50 to -69
Copy link
Contributor

Choose a reason for hiding this comment

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

So we get the font size stuff out of the box with __experimentalFontSize? Yay! 🎉

Copy link
Contributor Author

@Addison-Stavlo Addison-Stavlo Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah! Actually, a lot of that goo you highlighted I had to add when I added the __experimentalFontSize but was still using the HOC for colors and needed them to still 'play nice'. Now that we are using the flag for this color hook as well things work much better out of the box and all this gunk is no longer seems required.

const ref = useRef();
const {
TextColor,
BackgroundColor,
InspectorControlsColorPanel,
ColorPanel,
} = __experimentalUseColors(
[
{ name: 'textColor', property: 'color' },
{ name: 'backgroundColor', className: 'background-color' },
],
{
contrastCheckers: [
{
backgroundColor: true,
textColor: true,
fontSize: contrastCheckFontSize,
},
],
colorDetector: { targetRef: ref },
colorPanelProps: {
initialOpen: true,
},
},
[ contrastCheckFontSize ]
);

const { align, showAvatar, showBio, byline } = attributes;

const avatarSizes = [];
Expand Down Expand Up @@ -162,64 +111,51 @@ function PostAuthorEdit( { isSelected, context, attributes, setAttributes } ) {
</PanelBody>
</InspectorControls>

{ InspectorControlsColorPanel }

<BlockControls>
<AlignmentToolbar
value={ align }
onChange={ ( nextAlign ) => {
setAttributes( { align: nextAlign } );
} }
/>
<BlockColorsStyleSelector
TextColor={ TextColor }
BackgroundColor={ BackgroundColor }
>
{ ColorPanel }
</BlockColorsStyleSelector>
</BlockControls>

<TextColor>
<BackgroundColor>
<div ref={ ref } className={ classNames.block }>
{ showAvatar && authorDetails && (
<div className="wp-block-post-author__avatar">
<img
width={ attributes.avatarSize }
src={
authorDetails.avatar_urls[
attributes.avatarSize
]
}
alt={ authorDetails.name }
/>
</div>
) }
<div className="wp-block-post-author__content">
{ ( ! RichText.isEmpty( byline ) ||
isSelected ) && (
<RichText
className="wp-block-post-author__byline"
multiline={ false }
placeholder={ __( 'Write byline …' ) }
value={ byline }
onChange={ ( value ) =>
setAttributes( { byline: value } )
}
/>
) }
<p className="wp-block-post-author__name">
{ authorDetails?.name }
</p>
{ showBio && (
<p className="wp-block-post-author__bio">
{ authorDetails?.description }
</p>
) }
</div>
<div className={ classNames.block }>
{ showAvatar && authorDetails && (
<div className="wp-block-post-author__avatar">
<img
width={ attributes.avatarSize }
src={
authorDetails.avatar_urls[
attributes.avatarSize
]
}
alt={ authorDetails.name }
/>
</div>
</BackgroundColor>
</TextColor>
) }
<div className="wp-block-post-author__content">
{ ( ! RichText.isEmpty( byline ) || isSelected ) && (
<RichText
className="wp-block-post-author__byline"
multiline={ false }
placeholder={ __( 'Write byline …' ) }
value={ byline }
onChange={ ( value ) =>
setAttributes( { byline: value } )
Copy link
Member

Choose a reason for hiding this comment

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

I imagine this is unrelated to the PR, but it is really weird that the byline is not persisted across all instances of the same author!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... it was set up as an attribute specific to the block for whatever reason. The bio on the other hand is per author.

Copy link
Member

Choose a reason for hiding this comment

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

interesting :p

}
/>
) }
<p className="wp-block-post-author__name">
{ authorDetails?.name }
</p>
{ showBio && (
<p className="wp-block-post-author__bio">
{ authorDetails?.description }
</p>
) }
</div>
</div>
</>
);
}
Expand Down
41 changes: 36 additions & 5 deletions packages/block-library/src/post-author/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ function post_author_build_css_colors( $attributes ) {

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is also a good opportunity to implement this #22872 (comment)

The idea being that you don't need to touch any backend or frontend code when applying the "useColors" flag, everything should just work.

Copy link
Contributor

@ockham ockham Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@youknowriad To clarify, you're suggesting to basically get #22872 #21420 merged (with a change to make the gutenberg_apply_style_attribute watch out for the supports flag, rather than just individual styling related attributes), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think #22872 implements any of that server side support for these hooks, so I guess the suggestion is to implement the render_block filter in this PR to add that support to __experimentalColor ? ( I don't totally follow the implementation details for that yet, but this would be something that would need to be set up for __experimentalColors, __expermentalFontSize, etc. separately? )

The idea being that you don't need to touch any backend or frontend code when applying the "useColors" flag, everything should just work.

That would be great! But currently we also have to touch the backend to inject the classNames and styles for __experimentalFontSize and alignment as well. It sounds like these need to be updated in the same way? Maybe we should handle all of these being updated in a separate PR?

But I agree, it would be really nice to implement them. Having to go through this manually in index.php is definitely a pain we would like to avoid.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I meant #21420 rather than #22872, fixed my original comment now.)

That would be great! But currently we also have to touch the backend to inject the classNames and styles for __experimentalFontSize and alignment as well. It sounds like these need to be updated in the same way? Maybe we should handle all of these being updated in a separate PR?

The way I was reading Riad's comment was to have a PR to cover all those flags in a render_blocks filter that's run regardless of block type 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@Addison-Stavlo I'm planning on working on this, do you want to team up to tackle the server side rendering? If we can get that done then we don't need to write any custom className and style code in any of these FSE blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was planning on using #23007 to add the code, so we have a block to test with at the same time.

Copy link
Contributor

@apeatling apeatling Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The way I was reading Riad's comment was to have a PR to cover all those flags in a render_blocks filter that's run regardless of block type 👍

One thing I'm unclear on is: should this function render classNames, inline block CSS, and global styles/css variables? I'm not clear on the relationship between your standard inline block CSS, and the new global styles using variables. /cc @youknowriad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the classes/styles for this one have already been manually set-up, I wouldn't be opposed to merging this and circling back to update it after we have enabled the proper support and updated the other FSE blocks on the task list.

Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, started work on the render function here https://github.com/WordPress/gutenberg/pull/23007/files#diff-1334f2cb73e38f2f89d2aaf763f07ed9R307

// Text color.
$has_named_text_color = array_key_exists( 'textColor', $attributes );
$has_custom_text_color = array_key_exists( 'customTextColor', $attributes );
$has_custom_text_color = array_key_exists( 'style', $attributes )
&& array_key_exists( 'color', $attributes['style'] )
&& array_key_exists( 'text', $attributes['style']['color'] );

// If has text color.
if ( $has_custom_text_color || $has_named_text_color ) {
Expand All @@ -37,15 +39,40 @@ function post_author_build_css_colors( $attributes ) {
$text_colors['css_classes'][] = sprintf( 'has-%s-color', $attributes['textColor'] );
} elseif ( $has_custom_text_color ) {
// Add the custom color inline style.
$text_colors['inline_styles'] .= sprintf( 'color: %s;', $attributes['customTextColor'] );
$text_colors['inline_styles'] .= sprintf( 'color: %s;', $attributes['style']['color']['text'] );
}

// Link colors.
$has_link_color = array_key_exists( 'style', $attributes )
&& array_key_exists( 'color', $attributes['style'] )
&& array_key_exists( 'link', $attributes['style']['color'] );

if ( $has_link_color ) {
$text_colors['css_classes'][] = 'has-link-color';
// If link is a named color.
if ( strpos( $attributes['style']['color']['link'], 'var:preset|color|' ) !== false ) {
// Get the name from the string and add proper styles.
$index_to_splice = strrpos( $attributes['style']['color']['link'], '|' ) + 1;
$link_color_name = substr( $attributes['style']['color']['link'], $index_to_splice );
$text_colors['inline_styles'] .= sprintf( '--wp--style--color--link:var(--wp--preset--color--%s);', $link_color_name );
} else {
$text_colors['inline_styles'] .= sprintf( '--wp--style--color--link: %s;', $attributes['style']['color']['link'] );
}
}
// Background color.
$has_named_background_color = array_key_exists( 'backgroundColor', $attributes );
$has_custom_background_color = array_key_exists( 'customBackgroundColor', $attributes );
$has_custom_background_color = array_key_exists( 'style', $attributes )
&& array_key_exists( 'color', $attributes['style'] )
&& array_key_exists( 'background', $attributes['style']['color'] );

// Gradient color.
$has_named_gradient = array_key_exists( 'gradient', $attributes );
$has_custom_gradient = array_key_exists( 'style', $attributes )
&& array_key_exists( 'color', $attributes['style'] )
&& array_key_exists( 'gradient', $attributes['style']['color'] );

// If has background color.
if ( $has_custom_background_color || $has_named_background_color ) {
if ( $has_custom_background_color || $has_named_background_color || $has_named_gradient || $has_custom_gradient ) {
// Add has-background-color class.
$background_colors['css_classes'][] = 'has-background-color';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im wondering if this should just be 'has-background'. I notice that __experimentalColor applies the 'has-background' class as opposed to the 'has-background-color' class:

gutenberg/packages/block-editor/src/hooks/color.js

Lines 130 to 133 in c53d36b

'has-background':
backgroundColor ||
style?.color?.background ||
( hasGradient && ( gradient || style?.color?.gradient ) ),

Im assuming either this should be changed to 'has-background', or the hook should be using 'has-background-color' ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely change this to has-background we should match __experimentalColor 💯

}
Expand All @@ -55,7 +82,11 @@ function post_author_build_css_colors( $attributes ) {
$background_colors['css_classes'][] = sprintf( 'has-%s-background-color', $attributes['backgroundColor'] );
} elseif ( $has_custom_background_color ) {
// Add the custom background-color inline style.
$background_colors['inline_styles'] .= sprintf( 'background-color: %s;', $attributes['customBackgroundColor'] );
$background_colors['inline_styles'] .= sprintf( 'background-color: %s;', $attributes['style']['color']['background'] );
} elseif ( $has_named_gradient ) {
$background_colors['css_classes'][] = sprintf( 'has-%s-gradient-background', $attributes['gradient'] );
} elseif ( $has_custom_gradient ) {
$background_colors['inline_styles'] .= sprintf( 'background: %s;', $attributes['style']['color']['gradient'] );
}

return array(
Expand Down