-
Notifications
You must be signed in to change notification settings - Fork 16
Description
What problem does this address?
A follow up to #17. This added server side filters to add custom services and render them.
Unfortunately - as pointed out by @philcable - in the editor the icon and label fallback to the email icon:
The icon and title of my variation are displaying correctly in the sidebar, but the Mail icon is displaying in the content area. This is due to the logic at https://github.com/ndiego/social-sharing-block/blob/main/src/social-sharing-link/social-list.js#L31 and https://github.com/ndiego/social-sharing-block/blob/main/src/social-sharing-link/social-list.js#L50, which falls back to the Mail block settings when the variation isn't found in the array provided in the variations file.
I know this plugin and blocks are heavily inspired by the core social-link block. They unfortunately have the same problem. Adding custom variations still falls back to the ChainIcon.
Important: This is not about the block icon - which the editor displays in the blocks toolbar, inspector controls and inserter. The block icon can be set when registering the variation via registerBlockVariation.
Testing instructions
- Register a custom block variation for the
outermost/social-sharing-linkblock in JavaScript (editor):
registerBlockVariation("outermost/social-sharing-link", {
name: "link",
isDefault: true,
title: __("Link"),
icon: "admin-links",
attributes: {
service: "link",
},
isActive: (blockAttributes, variationAttributes) => blockAttributes.service === "link",
});- Add a social sharing block in the editor and add this variation as a child.
- The block shows the mail icon by default.
What is your proposed solution?
1. Filters in icon/label functions:
Add filters to getIconBySite and getLabelBySite in social-list.js:
getLabelBySite:return applyFilters('social-sharing-block.link-label', label, name);getIconBySite:return applyFilters('social-sharing-block.link-icon', icon, name);
The filter for the icons need to return a renderable SVG/React component, so only passing a string (like the name of a Dashicon) is not possible. One has to import the icon from @wordpress/icons.
Problems:
Those two functions are called in the Edit function of the link block and therefore will be called on each re-render. That means that those filters will be called many times. I'm not quite sure yet, what the best practice about hooks (actions + filters) in react components that re-render often is. I know core uses some filters (like editor.BlockEdit).
2. Filter the complete variations list in variations.js
Instead of directly returning the array of variations in variations.js the file instead exports a function called getVariations in which the variations are passed through a filter (applyFilters('social-sharing-block.variations', variations);).
Problems:
A developer who wants to register a block variation for a custom service has to register their variation two times or either using the offical way or the filter here. That might be confusing.
Also during block variation the call to getVariations() might occur before plugins/themes have loaded and therefore hooks of those have not been added. So theire variations through this hook are not added.
In getLabelBySite and getIconBySite we can call getVariations each time (instead in the top at import-time, that will be to early as well). But than that function gets called on each render as well (see problems solution 1).
So not ideal either.
3. Use block variation icon and label
In the edit function always try to get the icon of the current rendered variation. getIconBySite actually re-uses the icon of the variation, but only for hardcoded "core" variations of the plugin. We can use getActiveBlockVariation from the block editor data store to get the active variation and use that icon. That automatically uses core's isActive filter to get the active variation and we don't have to do any mapping/searching by service name.
Puh, for what I thought would be simple problem/solution, I wrote a lot of text 😅🙈
I think I prefer solution no 3. I've tried out solution 1 and 2 in a local dev environment. If interested and you are willing to accept a PR, I can draft one up.