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

Social Link: group all variations under one block type #19887

Merged
merged 9 commits into from
Feb 6, 2020
Merged

Social Link: group all variations under one block type #19887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
febaaff
Social Link: group all variations under one block type
mcsf Feb 3, 2020
c5a0da2
Social Link: Update test fixtures
mcsf Feb 4, 2020
38eacaa
Address review feedback
mcsf Feb 5, 2020
52071b0
Fix misnamed attribute 'type' -> 'site'
mcsf Feb 5, 2020
ba9070f
Social Link: Rename attribute 'site' to 'service'
mcsf Feb 5, 2020
586da71
Social Link: Update description
mcsf Feb 5, 2020
74c7987
Change attribute name to service (previously site)
mkaz Feb 5, 2020
d1b6347
Lint: Equal sign alignment
mkaz Feb 6, 2020
07a9e36
Social Link: Remove unnecessary block deprecation
mcsf Feb 6, 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
2 changes: 1 addition & 1 deletion packages/block-library/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ export const __experimentalRegisterExperimentalCoreBlocks =
[
__experimentalEnableLegacyWidgetBlock ? legacyWidget : null,
socialLinks,
...socialLink.sites,
socialLink,

// Register Full Site Editing Blocks.
...( __experimentalEnableFullSiteEditing
Expand Down
15 changes: 15 additions & 0 deletions packages/block-library/src/social-link/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"name": "core/social-link",
Copy link
Member

Choose a reason for hiding this comment

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

I'm also wondering now that I noticed that the block's title is now Social Icon, should we change the name as well? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I started this PR I was pretty much on the links camp, but now I lean a bit towards icon too.

Copy link
Member

Choose a reason for hiding this comment

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

@mcsf Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I was initially considering the primary function of these blocks to be the linking — and this fit in with the possibility of declaring Social Links as a variation of Navigation or Buttons. But now I think we may compromise on functionality if we center the semantics of these blocks around just the linking.

Instead, their real purpose lies more in the appearance: the rendering of the links as icons. In the future, the icons may change looks, colours or shape, but they will remain icons. I think the user is best served if we are clear about this. If, instead, the primary purpose is linking, then other blocks are better suited (Navigation, Buttons, File, etc.).

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 have a diff that moves everything to Icons, but it's very large, so I'll open a separate PR to discuss this.

Copy link
Member

Choose a reason for hiding this comment

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

Sure thing 💯

Copy link
Member

Choose a reason for hiding this comment

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

The point of having social icon links is that they're links, not that they're icons, isn't it? From an accessibility standpoint, I question whether we should even have a block dedicated to textless buttons in the first place. Wouldn't that be sort of an anti-pattern?

Also, what if I want to have social links with visible text labels? Would I use the Buttons block for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of having social icon links is that they're links, not that they're icons, isn't it?

That's the subject part of the debate, I think.

From an accessibility standpoint, I question whether we should even have a block dedicated to textless buttons in the first place. Wouldn't that be sort of an anti-pattern?

If done correctly, none of it should matter: icons should be as accessible as a regular text link, whether the reader cannot see, cannot use a cursor, etc.

Also, what if I want to have social links with visible text labels? Would I use the Buttons block for that?

I could see either block type accommodating that use case.

I'll cc you on the new PR if you're interested in discussing this there.

"category": "widgets",
"attributes": {
"url": {
"type": "string"
},
"service": {
"type": "string"
},
"label": {
"type": "number"
}
}
}
8 changes: 4 additions & 4 deletions packages/block-library/src/social-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ import { __, sprintf } from '@wordpress/i18n';
import { getIconBySite, getNameBySite } from './social-list';

const SocialLinkEdit = ( { attributes, setAttributes, isSelected } ) => {
const { url, site, label } = attributes;
const { url, service, label } = attributes;
const [ showURLPopover, setPopover ] = useState( false );
const classes = classNames( 'wp-social-link', 'wp-social-link-' + site, {
const classes = classNames( 'wp-social-link', 'wp-social-link-' + service, {
'wp-social-link__is-incomplete': ! url,
} );

// Import icon.
const IconComponent = getIconBySite( site );
const socialLinkName = getNameBySite( site );
const IconComponent = getIconBySite( service );
const socialLinkName = getNameBySite( service );

return (
<Fragment>
Expand Down
41 changes: 12 additions & 29 deletions packages/block-library/src/social-link/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,40 +7,23 @@ import { __ } from '@wordpress/i18n';
* Internal dependencies
*/
import edit from './edit';
import socialList from './social-list';
import metadata from './block.json';
import variations from './variations';

const commonAttributes = {
category: 'widgets',
const { name } = metadata;

export { metadata, name };

export const settings = {
title: __( 'Social Icon' ),
parent: [ 'core/social-links' ],
supports: {
reusable: false,
html: false,
},
edit,
description: __(
'Display an icon linking to a social media profile or website.'
),
variations,
};

// Create individual blocks out of each site in social-list.js
export const sites = Object.keys( socialList ).map( ( site ) => {
const siteParams = socialList[ site ];
return {
name: 'core/social-link-' + site,
settings: {
title: siteParams.name,
icon: siteParams.icon,
description: __( 'Link to ' + siteParams.name ),
...commonAttributes,
attributes: {
url: {
type: 'string',
},
site: {
type: 'string',
default: site,
},
label: {
type: 'string',
},
},
},
};
} );
42 changes: 23 additions & 19 deletions packages/block-library/src/social-link/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,17 @@
* @return string Rendered HTML of the referenced block.
*/
function render_core_social_link( $attributes ) {
$site = ( isset( $attributes['site'] ) ) ? $attributes['site'] : 'Icon';
$url = ( isset( $attributes['url'] ) ) ? $attributes['url'] : false;
$label = ( isset( $attributes['label'] ) ) ? $attributes['label'] : __( 'Link to ' ) . core_social_link_get_name( $site );
$service = ( isset( $attributes['service'] ) ) ? $attributes['service'] : 'Icon';
$url = ( isset( $attributes['url'] ) ) ? $attributes['url'] : false;
$label = ( isset( $attributes['label'] ) ) ? $attributes['label'] : __( 'Link to ' ) . core_social_link_get_name( $service );

// Don't render a link if there is no URL set.
if ( ! $url ) {
return '';
}

$icon = core_social_link_get_icon( $site );
return '<li class="wp-social-link wp-social-link-' . $site . '"><a href="' . esc_url( $url ) . '" aria-label="' . esc_attr( $label ) . '"> ' . $icon . '</a></li>';
$icon = core_social_link_get_icon( $service );
return '<li class="wp-social-link wp-social-link-' . $service . '"><a href="' . esc_url( $url ) . '" aria-label="' . esc_attr( $label ) . '"> ' . $icon . '</a></li>';
}

/**
Expand Down Expand Up @@ -72,26 +72,30 @@ function register_block_core_social_link() {
'youtube',
);

$path = __DIR__ . '/social-link/block.json';
$metadata = json_decode( file_get_contents( $path ), true );

foreach ( $sites as $site ) {
register_block_type(
'core/social-link-' . $site,
array(
'attributes' => array(
'url' => array(
'type' => 'string',
),
'site' => array(
'type' => 'string',
'default' => $site,
),
'label' => array(
'type' => 'string',
),
),
'render_callback' => 'render_core_social_link',
array_merge(
$metadata,
array(
'render_callback' => 'render_core_social_link',
)
)
);
}

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'd love to get rid of the loop above right away, in which all core/social-link-<foo> types are generated, but in order to do that we need a better plan for back-compat.

Right now only the editor has a back-compat provision via the change in packages/blocks/src/api/parser.js. How could we best handle this for server-side block rendering for the front-end?

Copy link
Member

Choose a reason for hiding this comment

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

You should check the HTML output of the edit post page. All blocks registered here will have a huge impact on the size of the page. In particular, the part that registers blocks to work with the ServerSideRender component (which is also pointless in this case as they will never be consumed).

At the same time, it's something that was never part of the WordPress core so we can provide the backward-compatibility only for the Gutenberg plugin and call it the day 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the same time, it's something that was never part of the WordPress core so we can provide the backward-compatibility only for the Gutenberg plugin and call it the day 😄

Yeah, that seems like the right way to go. Let's address in a subsequent PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#20098

register_block_type(
$metadata['name'],
array_merge(
$metadata,
array(
'render_callback' => 'render_core_social_link',
)
)
);
}
add_action( 'init', 'register_block_core_social_link' );

Expand Down
Loading