-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Icons: Ship all SVG icons to WordPress core #79102
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
base: icons/snake-case
Are you sure you want to change the base?
Changes from all commits
f1c8cd1
75ee847
8855ede
9fdaf8b
617f209
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| https://github.com/WordPress/wordpress-develop/pull/12151 | ||
|
|
||
| * https://github.com/WordPress/gutenberg/pull/79102 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,4 +15,47 @@ | |
| * @since 7.1.0 | ||
| */ | ||
| class WP_REST_Icons_Controller_Gutenberg extends WP_REST_Icons_Controller { | ||
| /** | ||
| * Retrieves all icons. | ||
| * | ||
| * @param WP_REST_Request $request Full details about the request. | ||
| * @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure. | ||
| */ | ||
| public function get_items( $request ) { | ||
| $response = array(); | ||
| $search = $request->get_param( 'search' ); | ||
| $icons = WP_Icons_Registry::get_instance()->get_registered_icons( $search ); | ||
| foreach ( $icons as $icon ) { | ||
| if ( empty( $icon['show_in_rest'] ) ) { | ||
| continue; | ||
| } | ||
| $prepared_icon = $this->prepare_item_for_response( $icon, $request ); | ||
| $response[] = $this->prepare_response_for_collection( $prepared_icon ); | ||
| } | ||
| return rest_ensure_response( $response ); | ||
| } | ||
|
|
||
| /** | ||
| * Retrieves a specific icon from the registry. | ||
| * | ||
| * @param string $name Icon name. | ||
| * @return array|WP_Error Icon data on success, or WP_Error object on failure. | ||
| */ | ||
| public function get_icon( $name ) { | ||
| $icon = parent::get_icon( $name ); | ||
|
|
||
| if ( ! is_wp_error( $icon ) && empty( $icon['show_in_rest'] ) ) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also needs a test IMO.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, what's the scenario to this path of the code? Won't |
||
| return new WP_Error( | ||
| 'rest_icon_not_found', | ||
| sprintf( | ||
| // translators: %s is the name of any user-provided name | ||
| __( 'Icon not found: "%s".', 'gutenberg' ), | ||
| $name | ||
| ), | ||
| array( 'status' => 404 ) | ||
| ); | ||
| } | ||
|
|
||
| return $icon; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,8 +68,9 @@ protected function __construct() { | |
| $this->register( | ||
| 'core/' . $icon_name, | ||
| array( | ||
| 'label' => $icon_data['label'], | ||
| 'file_path' => $icons_directory . $icon_data['filePath'], | ||
| 'label' => $icon_data['label'], | ||
| 'file_path' => $icons_directory . $icon_data['filePath'], | ||
| 'show_in_rest' => ! empty( $icon_data['showInRest'] ), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add test for the |
||
| ) | ||
| ); | ||
| } | ||
|
|
@@ -87,6 +88,7 @@ protected function __construct() { | |
| * If not provided, the content will be retrieved from the `file_path` if set. | ||
| * If both `content` and `file_path` are not set, the icon will not be registered. | ||
| * @type string $file_path Optional. The full path to the file containing the icon content. | ||
| * @type bool $show_in_rest Optional. Whether the icon is exposed through the REST API. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that spacing will need to be aligned here. |
||
| * } | ||
| * @return bool True if the icon was registered with success and false otherwise. | ||
| */ | ||
|
|
@@ -100,7 +102,7 @@ protected function register( $icon_name, $icon_properties ) { | |
| return false; | ||
| } | ||
|
|
||
| $allowed_keys = array_fill_keys( array( 'label', 'content', 'file_path' ), 1 ); | ||
| $allowed_keys = array_fill_keys( array( 'label', 'content', 'file_path', 'show_in_rest' ), 1 ); | ||
| foreach ( array_keys( $icon_properties ) as $key ) { | ||
| if ( ! array_key_exists( $key, $allowed_keys ) ) { | ||
| _doing_it_wrong( | ||
|
|
@@ -158,6 +160,15 @@ protected function register( $icon_name, $icon_properties ) { | |
| } | ||
| } | ||
|
|
||
| if ( isset( $icon_properties['show_in_rest'] ) && ! is_bool( $icon_properties['show_in_rest'] ) ) { | ||
| _doing_it_wrong( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test for this too? |
||
| __METHOD__, | ||
| __( 'Icon show_in_rest flag must be a boolean.', 'gutenberg' ), | ||
| '7.1.0' | ||
| ); | ||
| return false; | ||
| } | ||
|
|
||
| $icon = array_merge( | ||
| $icon_properties, | ||
| array( 'name' => $icon_name ) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,10 +45,12 @@ function generatePHPArray( manifest ) { | |
| const key = formatPHPKey( item.slug, maxKeyLength ); | ||
| const label = escapePHPString( item.label ); | ||
| const filePath = escapePHPString( item.filePath ); | ||
| const showInRest = item.showInRest ? 'true' : 'false'; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noting that the icon manifest obviously grew a lot with this change. That's likely fine on its own, but I'm also considering the changes in #79100 where we're now essentially hydrating the content for every icon whose content is on the filesystem. That might mean hundreds of disk I/O operations; is it possible this will lead to a performance regression? I'm not sure what the solution is, but maybe it makes sense to consider lazy hydrating the icon content, or maybe introducing some filtering so we don't call |
||
|
|
||
| return `${ key } => array( | ||
| 'label' => _x( '${ label }', 'icon label', 'gutenberg' ), | ||
| 'filePath' => '${ filePath }', | ||
| 'label' => _x( '${ label }', 'icon label', 'gutenberg' ), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When are we actually going to surface the translations for icons that we don't show in the REST API? It seems a bit wasteful to localize all those strings and add hundreds of new non-translated strings to core if we're never going to surface them to end users. |
||
| 'filePath' => '${ filePath }', | ||
| 'showInRest' => ${ showInRest }, | ||
| ),`; | ||
| } ); | ||
|
|
||
|
|
@@ -59,20 +61,18 @@ ${ phpEntries.join( '\n' ) } | |
|
|
||
| /** | ||
| * Generates manifest.php from manifest.json. | ||
| * Only includes icons with public: true. | ||
| */ | ||
| async function generateManifestPHP() { | ||
| const manifestJson = await readFile( MANIFEST_JSON_PATH, 'utf8' ); | ||
| const manifest = JSON.parse( manifestJson ); | ||
| const publicIcons = manifest.filter( ( item ) => item.public === true ); | ||
| const phpHeader = `<?php | ||
| // This file is automatically generated. Do not edit directly. | ||
| if ( ! defined( 'ABSPATH' ) ) { | ||
| die( 'Silence is golden.' ); | ||
| } | ||
|
|
||
| `; | ||
| const phpArray = generatePHPArray( publicIcons ); | ||
| const phpArray = generatePHPArray( manifest ); | ||
| const phpContent = phpHeader + phpArray + '\n'; | ||
| await writeFile( MANIFEST_PHP_PATH, phpContent, 'utf8' ); | ||
| } | ||
|
|
||
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 seems to be fully reimplementing the parent class method. Should we try to reuse it as much as possible and just sprinkle the
show_in_restfilter logic on top?