-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 Links: Add filter to services data to enable extendibility #30749
base: trunk
Are you sure you want to change the base?
Conversation
Size Change: +23 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
81d4e3e
to
3b127a7
Compare
In a previous PR we added a "templateLock" attribute to the "column" block to be able to edit it there. In a lot of ways "templateLock" is similar to "allowedBlocks" and "template". An attribute allows templates to override these things for specific instances which is arguably more powerful. I'm personally not at ease with using filters here (or any js filter for the record), it breaks too often depending on how/when scripts are loaded, components rerendered... |
@youknowriad
I'll test out as an attribute, because then a plugin developer can override the setting using a register filter and achieve a similar result. |
3b127a7
to
583d6fb
Compare
Updated, to use an attribute. I also updated the PR and testing instructions to match. |
Using attributes to list the allowed blocks makes sense to me. And it is filterable through the existing However, how would you proceed when the list is dynamically generated like in gutenberg/packages/block-library/src/social-links/edit.native.js Lines 22 to 24 in 2190665
Does it need to be dynamically generated in the first place? It is not obvious to me why the |
I like the refined proposal after the feedback shared by @youknowriad. It's possible to change the list of default allowed attributes through |
"allowedBlocks": { | ||
"type": "array", | ||
"default": [ "core/social-link" ] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if this might be redundant since the block core/social-link
will be always allowed because its parent is this block. In fact, I tried to set the allowedBlocks
to an empty array and I could still see all the social link variants displayed in the inserter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I completely forgot about it. It would be great to investigate how those two features interact with each other. The biggest limitation of parent
is that it won't work with more than one level of nesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I tried fiddling with the parent
property, and so far I am able to insert a custom block from a third-party plugin just by setting it a parent
attribute.
Since child blocks are only available through their parent blocks, we could simply register two blocks that uses the same components for a block to be both available as an inner block and as a 'standalone' block.
For the nesting level concern, may it be something we want? Let's say third-plugin A adds its a/social-share-link
in core/social-link
inner blocks, while third-party plugin B adds its own b/social-profile-avatar-display
which accepts an inner core/social-link
block, and retrieves the profile picture from the passed social link (throughout some CORS bypass black magic).
Wouldn't we, in this case, want to prevent the a/social-share-link
to be available as an inner block of b/social-profile-avatar-display
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest limitation of parent is that it won't work with more than one level of nesting.
Yeah, good point. Are the allowed blocks list passed through allowedBlocks
prop propagated to nested children? I haven't checked this further but it looks like it doesn't neither, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a great catch because I was testing Navigation with the same update and trying to limit the list of blocks and hadn't figured out yet why it was still showing the other blocks.
👋 I'm not sure how this fixes the parent issue, #19041. Is the goal to be able to insert third-party icon-like blocks alongside Allowing new child types inside Social Links would lead to a more fragmented user experience, where some of the icons no longer behave identically — the toolbar would shift, as would the block settings UI. Also, we would miss out on Gutenberg's multi-select feature that lets the user set attributes for multiple blocks of the same type. |
Thanks @raaaahman for pointing this out! I think we don't really need to dynamically generate them, I'm not sure what was the purpose of doing it this way, I checked the PR that introduced it but I didn't get any clue regarding this. I ran some tests and I found out that it doesn't look like it's necessary to pass the social link variants in the Here I reference some code pieces related to this:
EDIT: Unless there's any potential issue I might be overlooking, I think we could apply the same logic in the native version of the |
I can't remember exactly why this decision was made but I believe it was mostly UX decisions, some things that come to mind:
I'm sure we could solve this in another way if we want to remove this custom |
Thanks @geriux for the insights! That was my impression too, it looks like the way the native version is registering the block variants is related to what you commented. For this case what I suggest is only to modify the way the allowed blocks list is calculated in the Suggested change
One thing I'd like to note is that the |
@tomjn Thanks for the ping Tom, I've got this tab open and will follow up and make sure it's ready. I believe when I last left it was but can read through it all again and see if anything has changed. |
Is code from above is still working? Unfortunately I have problem with proper icon - in elements browser looks good, but the rendered icon is wrong. js:
php:
results: Is Tested at Wordpress v.5.8.2 |
@BeholdPL are you testing the code with this PR? |
Tested on WP 5.8.2 |
Then there's the problem, the code in this pull request is intended for testing this pull request, not as an example for general use. This PR has not been merged. To use this code, you would need to pull down this branch of the gutenberg plugin, build it, and activate it. There is no guarantee it will work, or that the PR does not need additional work. It is not a copy pasta example for common use. |
Any results you have gotten in 5.8.2 are accidental/coincidences. I know that the filter proposed here is in use in the twentytwentyone theme for use on the frontend though with slight differences |
Got it. As far as I know there is no way for extending this block in current WP version (5.8.2), isn't it? |
You can try the Patched Block: https://gist.github.com/mkkeck/84fe5fda939002fcf210d66b99de7c0f |
@mkkeck How can I use this in my WordPress website without the need to be afraid for updated? :) |
Use it simply as a plugin. |
I'm sorry to ask @mkkeck, but I can't get it working. Could you share your full plugin with social-services.json file by any change? Sorry for asking! |
Please take look at lines #84 until #97:
I think you have to replace it with:
|
Thanks to Cameron Jones ( https://cameronjonesweb.com.au/blog/adding-a-custom-icon-to-the-social-links-block-should-not-be-this-hard/ ) I am able to add TripAdvisor icon as well.
and the block;
hope it helps to someone else too. |
I've updated the ghist: Simply copy the code and put it into a new file (php class) under your child theme. Created a json file with all your icons and save it as social-services.json under your child theme: Example json data:
|
I suspect with the recent launch of a new social media site (Threads) that a lot of folks will end up in this PR trying to figure out why it's still so awkward to add a new icon to the social links block. All I really want is a filter hook added to Edit: I suppose that doesn't take care of the icon rendered in the block in the editor, but that looks to also be addressed in this PR. |
Here I am, back again, trying to add an icon for Bluesky. Still have to hack it in, and still have no good way of displaying the icon correctly inside the editor. This is after an icon for Threads has been added in #52685, in the comments of which @mtias says that core should maintain the social icons. We can already register a block variation, and we can show the correct SVG on the front end by filtering the block render (not a great solution, but it works). The only thing that doesn't have a clear workaround (at least in this thread) is showing the correct icon in the editor. If we can get this far, people are going to do it. I'm doing it to include icons core doesn't support on my own site. I'm sure there are plugins already doing all sorts of wacky things to add more icons. We're just looking for some extensibility for a feature that is begging to be extended, as social platforms pop up all the time. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @mkaz, @raaaahman, @gwwar, @BeholdPL, @mkkeck, @NickHeurter, @sgeray, @KoiKoneessa, @YogieAnamCara, @aksthelion, @bshuchter, @davidallenlewis, @chs-kasvu, @hundkurserforalla. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I want to add my voice to this. There will always be social platforms / links that core won't provide - either because they are niche-platforms or because they are very regional/local. Another example: I like to use the social links block to add an icon to download the app and use a custom icon + link there. @ndiego social sharing link is actually pretty similar to the core social-link one, and he introduced custom icons in a recent commit (ndiego/social-sharing-block#23). I think the way this should work is pretty simple:
I don't think the social icons block needs to be a full-fledged icons block (although in the future, that is absolutely interesting). But I think as a first step, using the variations and adding a PHP filter is a quick fix for this. And something that should have been in there since the beginning - unfortunately, a lot of core blocks are not easily extendable. Before I saw this PR I even worked on my own PR for this issue, based on the code from the social-sharing-block. You can find it here: What are the blockers here atm? Is this a discussion about registering custom icons or are there concrete bugs with the implementation in the React Native side? |
Some pragmatic thoughts:
Note that this doesn't mean I do not support this, just that there are practical hurdles to overcome that aren't being discussed as much as they should. All of these are issues that need to be solved.
My understanding of block variations is that this is already the case, when defining your variation you can specify its icon. This is how all the |
This does not work. Specifically we're expecting the icon for the block variation to be used within the actual block displayed in the editor (not just the block selection list), which is how the core icons are displayed, but it's coming from a hardcoded list of variations.
This is very backwards-compatible and defaults to the generic link icon as your edit notes. Copy-pasting blocks with custom variations is already not going to work, why should this block be special? |
I even think we could do without the custom/new filter on rendering, if instead the variations are registered on the server and used for rendering as well. |
What is the status on this? |
Description
Enable the ability to extend Social Links.
Adds a PHP filter to social links services to add a definition for a link and SVG.
Fixes #19041
How has this been tested?
Create a plugin with that adds a filter to extend the services list, defining an icon and SVG for render side
Add JS that extends the variant and adds the icon for editor side.
Example
PHP (tester.php)
and JS (tester.js)
Types of changes