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

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

Merged
merged 9 commits into from
Feb 6, 2020

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Jan 24, 2020

Fixes #17277.

Background

Initially, Social Links were defined (#16897) as a multitude of block types (core/social-link-spotify, core/social-link-twitter) all based on a mold of a single social link. The block types would allow only one parent, the container core/social-links. From a user standpoint this works, but there are downsides:

  • In practice, this meant that the number of block types would grow with Social Links' number of supported sites.
  • The document semantics arguably become too specialised, since <!-- wp:social-link {"site":"wordpress","url":"https://example.org"} /--> is descriptive and more generalisable.
  • Because Social Links are currently defined as server-rendered blocks, there is overhead in registering all these variants on the server before page render. Making Social Links static is a discussion to be had in Social Links: Update SVG rendering to use CSS and background mask #19888.
  • From the previous point, it follows that REST API responses for the block type endpoints also increase in size to accommodate the generated block types.

Description

Recently, the idea of Block Variations (#16283) has solidified (#19243). Initially oriented towards patterns of (inner) blocks, Variations can now accommodate more varied scenarios. One such scenario is that of Social Links.

This PR reimplements Social Links as made up one parent, core/social-links, and one child, core/social-link. The child then defines a number of variations, one per supported site. This represents no noticeable difference for the user, except for the change in the content markup:

Before After
<!-- wp:social-link-wordpress {"url":"https://example.org"} /--> <!-- wp:social-link {"url":"https://example.org","site":"wordpress"} /-->

How has this been tested?

Screenshots

social-links-block-variations

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks great. Is there anything missing in the API?

I guess you should use store to get patterns to make this block extensible.

category: 'widgets',
parent: [ 'core/social-links' ],
supports: {
reusable: false,
html: false,
},
edit,
description: __( 'Link to a social media profile' ),
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'm not very happy with this description, as the patterns offer many kinds of Web-based resources: social media profiles, multimedia content, misc. pages.

@@ -92,6 +92,24 @@ function register_block_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.

attributes: { site: 'wordpress' },
title: 'WordPress',
icon: WordPressIcon,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unlike the Embed block, the Social Link block was initially conceived so that it can't be "generic", i.e. it has to follow a pattern. Given its purpose, this makes sense.

On the other hand, the way that Block Variations / Patterns are currently defined, if the "base" type is to be supplanted by the patterns there needs to be a default one. I chose WordPress as the least opinionated one (or the least controversial opinion?).

I guess we could instead adapt the Patterns API so that it's possible to supplant the base type without requiring a default, but I'm not sure it's worth adding that compromise. What do you think, @gziolo and @mtias?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side note: gotta love Vim for how magically it was able to transform social-list.js into patterns.js with just a quick macro. <3

@mcsf mcsf added [Block] Social Affects the Social Block - used to display Social Media accounts [Feature] Blocks Overall functionality of blocks [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible labels Jan 27, 2020
@mcsf mcsf changed the title WIP: Social Link: group all variations under one block type Social Link: group all variations under one block type Jan 27, 2020
@mcsf
Copy link
Contributor Author

mcsf commented Jan 27, 2020

I guess you should use store to get patterns to make this block extensible.

(edited after a DM exchange) Good point, thanks.

@mcsf
Copy link
Contributor Author

mcsf commented Jan 27, 2020

It looks great. Is there anything missing in the API?

Right now the only thing that comes to mind is #19887 (comment)

@mkaz
Copy link
Member

mkaz commented Jan 27, 2020

I'm wondering if Social Links should be just an Icon package for the logos, then the actual implementation of a social menu would look more like the Buttons block or Navigation block. This would do away with generating all of the individual site blocks.

Along the same lines, we could not have a special social links block and just add icon support to Buttons and/or Navigation.

@paaljoachim
Copy link
Contributor

Marcus that sounds very interesting!
Having the social blocks use the Buttons Block implementation seems like a good idea.
In a sense there could be two almost identical blocks. Buttons and Social. The user might find it easier having a Buttons Block and a Social Block, even though they are almost identical.
@jasmussen

@jasmussen
Copy link
Contributor

Along the same lines, we could not have a special social links block and just add icon support to Buttons and/or Navigation.

Purely on a technical and code quality level, you all should do what you need to do! If navigation, social links and buttons are technically a single block, cool with me. It even makes sense insofar as one might very well want both navigation items AND a couple of social link icons in the very same navigation block.

However from a user and block library discoverability perspective, though, it is important that Navigation, Social Links and Buttons remain three separate blocks, though, so that you can search the block library for either and insert it with confidence.

Like I said, it could be like the embed block where the YouTube block is literally the same block as the Embed block, just with a different name and icon. But it's important that they each have a separate entry in the block library. Why? This goes all the way back to the start:

The editor will endeavour to create a new page and post building experience that makes writing rich posts effortless, and has "blocks" to make it easy what today might take shortcodes, custom HTML, or "mystery meat" embed discovery.

Emphasis mine.

@gziolo
Copy link
Member

gziolo commented Jan 30, 2020

However from a user and block library discoverability perspective, though, it is important that Navigation, Social Links and Buttons remain three separate blocks, though, so that you can search the block library for either and insert it with confidence.

This is exactly what we discussed on WordPress Slack with @mkaz and @mcsf yesterday (link requires registration):
https://wordpress.slack.com/archives/C02QB2JS7/p1580308195116200

We want to make it possible with new block variations (formerly patterns) API.

@mkaz
Copy link
Member

mkaz commented Jan 31, 2020

For backward compatibility, can we add a deprecation to the social-links parent and use a migrate function which receives both attributes and InnerBlocks, and convert the InnerBlocks to the new social-link type. I think this can solve it in editor.

I'm not sure how we'd deprecate the server-side rendering

@mcsf mcsf force-pushed the try/social-link-block-variations branch from 134c2be to e4301bf Compare February 3, 2020 15:32
@mcsf mcsf changed the base branch from update/blocl-patterns-inserter-integration to master February 3, 2020 15:34
@mcsf
Copy link
Contributor Author

mcsf commented Feb 3, 2020

For backward compatibility, can we add a deprecation to the social-links parent and use a migrate function which receives both attributes and InnerBlocks, and convert the InnerBlocks to the new social-link type. I think this can solve it in editor.

This was solved early on by tackling the issue right in the parser, following an established pattern of renamed blocks (e.g. core/text).

@mcsf mcsf force-pushed the try/social-link-block-variations branch from 09caabf to 7ef5c7b Compare February 3, 2020 21:28
@mcsf mcsf force-pushed the try/social-link-block-variations branch from 7ef5c7b to 8125eba Compare February 4, 2020 16:13
@mcsf mcsf marked this pull request as ready for review February 4, 2020 19:15
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I haven't tested myself all this but code-wise it should be good to go once https://github.com/WordPress/gutenberg/pull/19887/files#r375784004 is addressed. It was one of the primary reasons why this block didn't land earlier in the WordPress core.

@@ -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.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I processed my previous comments and this is my conclusion, Let's land and improve block registration in PHP later. I don't see it as a blocker for the beta. In the plugin, it's going to work the same way anyway so we have a month to improve it :)

@mcsf mcsf merged commit cfc9cd4 into master Feb 6, 2020
@mcsf mcsf deleted the try/social-link-block-variations branch February 6, 2020 17:21
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 6, 2020
mcsf added a commit that referenced this pull request Feb 6, 2020
Fixes a regression introduced in #19887.

While changing the logic in `register_block_core_social_link` to consume
the block type's `block.json` and switch to the `$service` attribute
name, I broke the handling of the legacy format for social links.

Old format: <!-- wp:social-link-wordpress {"url":"foo"} /-->
New format: <!-- wp:social-link {"service":"wordpress","url":"foo"} /-->

This fixes the handling of legacy social links by providing the adequate
attribute default for each `$service`.
gziolo pushed a commit that referenced this pull request Feb 7, 2020
Fixes a regression introduced in #19887.

While changing the logic in `register_block_core_social_link` to consume
the block type's `block.json` and switch to the `$service` attribute
name, I broke the handling of the legacy format for social links.

Old format: <!-- wp:social-link-wordpress {"url":"foo"} /-->
New format: <!-- wp:social-link {"service":"wordpress","url":"foo"} /-->

This fixes the handling of legacy social links by providing the adequate
attribute default for each `$service`.
mcsf added a commit that referenced this pull request Feb 7, 2020
Since #19887, `core/social-link` was already titled "Social Icon". This
PR does the same to parent `core/social-links`.
mcsf added a commit that referenced this pull request Feb 10, 2020
Since #19887, `core/social-link` was already titled "Social Icon". This
PR does the same to parent `core/social-links`.
gziolo added a commit that referenced this pull request Feb 10, 2020
* Social Links: Consistently rebrand as Social Icons

Since #19887, `core/social-link` was already titled "Social Icon". This
PR does the same to parent `core/social-links`.

* Update packages/block-library/src/social-links/index.js

Co-Authored-By: Grzegorz (Greg) Ziółkowski <[email protected]>

* Update index.js

Co-authored-by: Grzegorz (Greg) Ziółkowski <[email protected]>
@jorgefilipecosta jorgefilipecosta added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Feb 11, 2020
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
23 tasks
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020
@mcsf mcsf mentioned this pull request Jul 30, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts [Feature] Blocks Overall functionality of blocks [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Social Links Block: Allow plugins to add additional logos/social links
9 participants