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

Add check for button text before rendering button block #29717

Merged
merged 1 commit into from
Mar 10, 2021
Merged

Add check for button text before rendering button block #29717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
100 changes: 100 additions & 0 deletions packages/block-library/src/button/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,15 @@ import classnames from 'classnames';
import {
RichText,
getColorClassName,
useBlockProps,
__experimentalGetGradientClass,
} from '@wordpress/block-editor';

/**
* Internal dependencies
*/
import getColorAndStyleProps from './color-props';

const migrateCustomColorsAndGradients = ( attributes ) => {
if (
! attributes.customTextColor &&
Expand Down Expand Up @@ -81,6 +87,100 @@ const blockAttributes = {
};

const deprecated = [
{
supports: {
anchor: true,
align: true,
alignWide: false,
color: {
__experimentalSkipSerialization: true,
},
reusable: false,
__experimentalSelector: '.wp-block-button__link',
},
attributes: {
...blockAttributes,
linkTarget: {
type: 'string',
source: 'attribute',
selector: 'a',
attribute: 'target',
},
rel: {
type: 'string',
source: 'attribute',
selector: 'a',
attribute: 'rel',
},
placeholder: {
type: 'string',
},
borderRadius: {
type: 'number',
},
backgroundColor: {
type: 'string',
},
textColor: {
type: 'string',
},
gradient: {
type: 'string',
},
style: {
type: 'object',
},
width: {
type: 'number',
},
},
save( { attributes, className } ) {
const {
borderRadius,
linkTarget,
rel,
text,
title,
url,
width,
} = attributes;
const colorProps = getColorAndStyleProps( attributes );
const buttonClasses = classnames(
'wp-block-button__link',
colorProps.className,
{
'no-border-radius': borderRadius === 0,
}
);
const buttonStyle = {
borderRadius: borderRadius ? borderRadius + 'px' : undefined,
...colorProps.style,
};

// The use of a `title` attribute here is soft-deprecated, but still applied
// if it had already been assigned, for the sake of backward-compatibility.
// A title will no longer be assigned for new or updated button block links.

const wrapperClasses = classnames( className, {
[ `has-custom-width wp-block-button__width-${ width }` ]: width,
} );

return (
<div { ...useBlockProps.save( { className: wrapperClasses } ) }>
<RichText.Content
tagName="a"
className={ buttonClasses }
href={ url }
title={ title }
style={ buttonStyle }
value={ text }
target={ linkTarget }
rel={ rel }
/>
</div>
);
},
},
{
supports: {
align: true,
Expand Down
26 changes: 14 additions & 12 deletions packages/block-library/src/button/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,17 +45,19 @@ export default function save( { attributes, className } ) {
} );

return (
<div { ...useBlockProps.save( { className: wrapperClasses } ) }>
<RichText.Content
tagName="a"
className={ buttonClasses }
href={ url }
title={ title }
style={ buttonStyle }
value={ text }
target={ linkTarget }
rel={ rel }
/>
</div>
text && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: you could have sed and early return here. Not that it changes anything, just wanted to share this code style trick in case you find it nicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!
I considered that when I saw the previous PR, but when I tried it, I got a lot of warnings/errors regarding unused variables (from everything included in the return that was skipped).

Is there a way to work around that, for future knowledge at least? I think this is pretty readable, but an early return is probably slightly better.

Copy link
Member

Choose a reason for hiding this comment

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

The answer is to just move the early return above the unused variables. I went ahead and made a PR for this: #29781.

Copy link
Contributor

@youknowriad youknowriad Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That PR is good. Though I would like to add just a small thing here. Early returns in components can break the "react hooks rules", so if we use this technique, we should make sure there's no React hook being called after the early return. (it's not the case here)

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad Good point. That kind of thing is caught by the React rules-of-hooks ESLint rules, isn't it? Or are we not using that?

Copy link
Contributor

Choose a reason for hiding this comment

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

you're probably right :)

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad that rule doesn't apply to custom hooks like useBlockProps?

Just want to know for future reference.

Copy link
Member

Choose a reason for hiding this comment

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

@Mamaduka useBlockProps.save() is not a hook. In fact, React hooks aren't even usable in a block's save implementation. It's actually just an alias that points to getBlockProps() from https://github.com/WordPress/gutenberg/blob/trunk/packages/blocks/src/api/serializer.js. Yeah, it's kinda weird.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @ZebulanStanphill for clarifying this.

<div { ...useBlockProps.save( { className: wrapperClasses } ) }>
<RichText.Content
tagName="a"
className={ buttonClasses }
href={ url }
title={ title }
style={ buttonStyle }
value={ text }
target={ linkTarget }
rel={ rel }
/>
</div>
)
);
}