[PoC] Add infrastracture to register icons #72080
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +210 B (+0.01%) Total Size: 2.03 MB
ℹ️ View Unchanged
|
Contributor
|
Thanks so much for getting this going, @t-hamano! Have you given any thought to how to integrate icons' ReactElement counterparts in this scenario? |
Contributor
Author
I have no ideas right now, but does this mean that we should allow icon registration on the client side as well? Example: registerIcon( {
title: 'Close',
name: 'core/close',
icon: () => (
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24">
<Path d="M19 6.41L17.59 5 12 10.59 6.41 5 5 6.41 10.59 12 5 17.59 6.41 19 12 13.41 17.59 19 19 17.59 13.41 12z" />
</SVG>
),
} ); |
Contributor
I wasn't really concerned with client-side registration. In my mind it was more about registering both the SVG source path and the // pseudocode in the editor
const { slug } = props
const iconData = await fetch( `/wp/v2/icons/${ slug }` )
const Icon = await import( iconData.reactSrc )
return <Icon />// pseudocode in render_callback
$content = WP_Icons_Registry::get_instance()->get_registered( $slug )['content']
return $contentMaybe this is an opportunity for Import Maps, maybe not. I'm not sure. Does this sound silly? |
Contributor
Author
|
I might not understand it exactly, but it seems like a very interesting approach. If you have concrete code, please feel free to push it to this branch. I consider this PR a playground. |
Contributor
Author
So, when a consumer registers an SVG icon string via PHP, does that mean that a React component file will also be automatically generated at the same time? If we expose Also, if we adopt your approach, the icon block will be dynamic, meaning it will only store the slug as an attribute, right? |
Contributor
|
I'll start with the last bit, because it's the one I'm most confident about:
Yes, at least that's how I see the Icon block. At least when the user selects an icon from a collection. But, when providing a custom icon via the block interface, maybe it's static.
Note that I too am very unsure of all this. :) This idea that icons could exist in a pair of SVG+ReactElement came from the original use case of the Icons package: to provide core icons for all our screens, and to remain "bundle-able" in a React project. This also started from a performance concern. Right now, the icons package is adequate for projects that use a limited set of statically imported icons, so that bundling can be done efficiently. But we'd like to avoid bundling the entire package in Also thinking of performance, maybe the fetching doesn't need to happen for individual icons, but by collection (where a collection often corresponds to a vendor/plugin). So, when the user edits an Icon block, we should be able to dynamically load n collections to display their icons in a grid. |
Contributor
Author
I came up with the following concept, what do you think? First, add a tool to the Developers using this tool can provide the path to the script file when registering the icon, as shown below: $icons_directory = plugin_dir_path( __FILE__ ) . '/icons/';
$svg_files = glob( $icons_directory . '*.svg' );
foreach ( $svg_files as $svg_file ) {
$icon_slug = basename( $svg_file, '.svg' );
$svg_content = file_get_contents( $svg_file );
$script_file_path = plugin_dir_path( __FILE__ ) . 'icons/' . $icon_slug . '.js';
$registry->register(
'my-plugin/' . $icon_slug,
array(
'svg' => $svg_content,
'scriptFilePath' => $script_file_path,
)
);
} |
Contributor
|
@t-hamano: that could work! That said... 😬 I feel like I've wasted your time today: I had a fresh look at this and had a nice conversation with @ellatrix, and now I believe that your approach with SVGs should be more than enough, and that trying to expose and import React elements is a big unnecessary complication.
The only other thing to consider is: should we keep icon registration closed to third parties for WP 6.9? I think so, so that we can more confidently try this out for core icons first. If you agree, do you have any ideas for limiting registration (other than simply annotating the function with |
Contributor
|
Riad also relayed this note:
But this is something that we can explore separately. Also cc @sgomes, who might be interested in the performance aspect. :) |
Contributor
Author
We may be able to make the <?php
final class WP_Icons_Registry {
public function __construct() {
$this->initialize_core_icons();
}
private function register( $icon_name, $icon_properties ) {
// ...
}
private function initialize_core_icons() {
$icons_directory = __DIR__ . '/../../packages/icons/src/library/';
$svg_files = glob( $icons_directory . '*.svg' );
foreach ( $svg_files as $svg_file ) {
$this->register(
// ...
);
}
}
} |
Contributor
|
Yes, that's exactly what I had in mind. So, overall, you agree with the plan? What do you think about the possible endpoint improvements? |
Contributor
Author
I fully support the plan, but I'm not sure we can implement it in 6.9 release 😅
I need help implementing the endpoints, as I may not fully understand them yet. Looking further into the future, I imagine the basic implementation for the icon API will be almost identical to the block pattern. That is,
|
Contributor
|
So, it's quite close for 6.9, but we can definitely strive for an MVP there. Our requirements are lower:
That should be enough for the Icon block to support:
|
Contributor
Author
|
Ok, I'll try to implement it tomorrow in a separate PR. |
Contributor
Contributor
Author
Contributor
|
@t-hamano: wonderful! I'll have a look. |
|
Flaky tests detected in 0a91fd0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/18378329335
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related:
This is a test PR to verify the construction of the infrastructure for icon registration.
WP_Icons_Registry
This class is very similar to
WP_Block_Patterns_Registry. For now, I registered all SVG icons that exist in@wordpress/iconspackage.Consumers can also register custom SVG icons like this:
Although not implemented in this PR, we might expose a utility function like the following:
WP_REST_Icons_Controller
Expose a list of icons registered via the icon registry.
Icon block
This is a block to test that the above two classes work and that the icon list can be referenced.