Call to screen_icon() in incorrect place in wp-admin/tools.php

[johnbillion]

screen_icon() should come after the opening <div class="wrap">

[johnbillion]

Patch

[azaozz]

(In [11940]) screen_icon() after <div class="wrap">, props johnbillion, fixes #10795

Companion PR to Gutenberg's ​https://github.com/WordPress/gutenberg/pull/72215 and ​https://github.com/WordPress/gutenberg/pull/74943

Builds on #10795

Trac ticket:

• Copy all icons, but check whether the public field is true in the icon registry.
• When copying icons, if the public field is not true, do not copy the icon. Additionally, remove icons not marked as 'public' => true from the manifest.php file.

Perhaps the latter approach is ideal if we don't want to ship internal icons entirely into core, but it may be a bit more complicated. What do you think?

@mcsf Since we're getting close to the Beta 1 release and the former approach is simpler, I’ve committed 970944b using that approach. I’d appreciate your thoughts.

@mcsf Since we're getting close to the Beta 1 release and the former approach is simpler, I’ve committed ​970944b using that approach. I’d appreciate your thoughts.

Sorry, I thought I had submitted a reply but apparently not. Yes, that's what I would prefer!

Update:

• 7b941e456b0c4e7e9603c13c6b8ee27f75cc8d82: I skipped the failing tests because there are no public icons in manifest.php yet. Once the Gutenberg commit hash is updated, we'll be able to re-enable the skipped tests. At first, I thought about registering a mock icon, but after reading ​this comment, I realized that that approach wasn't the right one.
• 15a38c31b97e5988027998e2319e05e48d1434f9: I added a ticket tag.

• ​7b941e4: I skipped the failing tests because there are no public icons in manifest.php yet. Once the Gutenberg commit hash is updated, we'll be able to re-enable the skipped tests. At first, I thought about registering a mock icon, but after reading ​this comment, I realized that that approach wasn't the right one.

Awesome, thanks. For the record, I just tested building WordPress with an updated Gutenberg ref, removed the tests' skip instructions, and I can confirm that the tests pass with the right manifest. 👍

I think we should be good to go. What else is missing for this initial commit?

It looks almost good, but I overlooked an important point: the text domain of the icon in the manifest.php file is gutenberg, so it will not be translatable in core. Would it be difficult to remove this text domain during the build process?

Great point. Will explore this after a call I'm on!

@mcsf, I think I've probably addressed all of that, but feel free to update!

@mcsf, I think I've probably addressed all of that, but feel free to update!

Oh, I didn't realise! I only realised you had already done this when I tried to push and had merge conflicts. 😅 In any case, thanks! Looks exactly like what I had.

Sorry for the confusion! But I think this PR is ready 👍

P.S. I'm planning to make some adjustments to how manifest.php works to avoid shipping unnecessary icon sets in core. I'll be submitting a PR in Gutenberg soon.

I wanted to get this big patch committed once and for all, but I have since reviewed ​your manifest.php PR, so this is a heads-up that I'll commit a follow-up to remove the ! ( $icon_data['public'] ?? false ) guard.

this is a heads-up that I'll commit a follow-up to remove the "! ( $icon_datapublic? ?? false )" guard.

Thank you! I was going to let you know that too 😄