Icons block: insert an icon by default#79111
Conversation
|
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 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. |
|
Size Change: +33 B (0%) Total Size: 8.58 MB 📦 View Changed
|
|
Flaky tests detected in 5d3c42c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27549334008
|
|
I'm going to push back on that a little, because I feel the social icons block while functional, is not a great user experience, in fact I could see us going in the other direction: converting the social icons block into a singular social icon block that inserts an icon by default and lets you change it. If you then want to group it in a container, you can pick a stack or a row, whatever you prefer. The gray material works reasonably for images because it is literally intended to be a placeholder. It works less well for icons, where it just looks like a tiny image, even when you select it. It isn't until you click the "Replace" button that you understand what the block is about, IMO. |
|
To elaborate more on the reason for why I think placeholder consistency is important, is that an icon can, conceptually, be an image block that renders a vector media file. Showing the same placeholder in both image and icon blocks benefit from conveying in the canvas that those are images. I took a quick look at #64288 and I see a good path in what you did in #66563. |
|
Appreciate that detective work, thank you. And I don't disagree that there's a path forward there. In this case I just feel like we're skipping a step: by picking an icon by default, the block is immediately useful and clear. The consistency argument is also important to always remember, but also here it's not a blanket rule. Some other blocks, like Button or Separator, they come in multiple variants, and yet also come with a default choice. I know that's a bit of a stretch in comparison. But the main thing is, image can't have a default, because it could be anything. That's the main reason it comes without a default. |
There was a problem hiding this comment.
I don't have a strong opinion on whether icons should be inserted by default, but for two reasons, we probably cannot completely remove the placeholder.
- There may be Icon blocks in a placeholder state where icons have not yet been inserted in the content. In this case, when the post editor is loaded, the icons should remain as placeholders rather than being set. Otherwise, it would unintentionally alter the user's content.
- Theme developers may want to place icon placeholders instead of actual icons in their templates or patterns.
| useEffect( () => { | ||
| if ( ! icon && allIcons?.length ) { | ||
| const randomIcon = | ||
| allIcons[ Math.floor( Math.random() * allIcons.length ) ]; | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| setAttributes( { icon: randomIcon.name } ); | ||
| } | ||
| }, [ | ||
| icon, | ||
| allIcons, | ||
| setAttributes, | ||
| __unstableMarkNextChangeAsNotPersistent, | ||
| ] ); |
There was a problem hiding this comment.
The default icon should only be applied to blocks that are being inserted. We should be able to use wasBlockJustInserted.
I haven't tested it, but it should look something like this.
const wasJustInserted = useSelect(
( select ) =>
select( blockEditorStore ).wasBlockJustInserted( clientId ),
[ clientId ]
);
const { __unstableMarkNextChangeAsNotPersistent } =
useDispatch( blockEditorStore );
useEffect( () => {
if ( wasJustInserted && ! icon && allIcons?.length ) {
const randomIcon =
allIcons[ Math.floor( Math.random() * allIcons.length ) ];
__unstableMarkNextChangeAsNotPersistent();
setAttributes( { icon: randomIcon.name } );
}
}, [
wasJustInserted,
icon,
allIcons,
setAttributes,
__unstableMarkNextChangeAsNotPersistent,
] );|
Thank you for the review, solid arguments. I do guess the main issue here is that we did ship this with the placeholder, therefore it exists, and removing it would be a breaking change. One thought is to designate an icon as the placeholder icon—an icon meant to be changed. It can be a simple bullet. I think I'll need to sleep more on whether that's a good idea, and also just generally invite more thoughts.
This one I don't know that I agree with; in both template and pattern design, some thought must be put into which icon could be a fit moreso than just "here is an icon". In that sense, forcing a choice is meant to actually improve those template and pattern designs. E.g. maps, addresses, opening hours, lists, there are good icons to choose from here, and a user can change them in the pattern if they like. The gray material being easily mistaken for an image placeholder, IMO, is the main problem to solve. |
I think we can just restore the deleted placeholder icon in this PR. Simply put, the icons that were placeholders remain placeholders. However, once an icon has been inserted, or for newly inserted icons, it is not possible to revert them back to placeholders. |
Newly inserted Icon blocks now render the info icon by default instead of a grey diagonal-line placeholder, so people see something that looks like an icon immediately. The placeholder SVG, its CSS rule, and the conditional render branch are dropped. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Replace the static "info" default with a random pick from the icon library so freshly inserted blocks show a different icon each time. The pick uses __unstableMarkNextChangeAsNotPersistent so it doesn't land in the undo stack — undoing past the random pick returns to the pre-insert state rather than cycling through new randoms. Co-Authored-By: Claude Opus 4.7 <[email protected]>
Co-Authored-By: Claude Opus 4.7 <[email protected]>
b698593 to
83a6b79
Compare
|
Okay, I've readded the placeholder. I've also removed the randomisation. I still think that was a fun addition, but to stay closer to existing metaphors, this one just inserts an info icon by default now. LMK your thoughts. |
Align the block example with the default icon so the inserter preview matches what users get on insert.
|
I went ahead and merged, because there's still time before the betas to revert or tweak, and I'm happy to follow up. Thanks all. |
| if ( ! icon && wasJustInserted ) { | ||
| // This side-effect should not create an undo level. | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| setAttributes( { icon: 'core/info' } ); |
There was a problem hiding this comment.
We could just declare core/info as the default icon value in attributes and add a migration. We should try to avoid similar side effects as much as possible; it teaches a bad pattern to consumers and anyone who reads core code.
@mcsf, I think you were working recently to remove similar patterns from blocks.
There was a problem hiding this comment.
Nice, let me know if you want me to follow up.
There was a problem hiding this comment.
I have another idea, which should be simpler and honor placeholder status more, IMO. I'll try to push PoC tomorrow.
There was a problem hiding this comment.
Thanks for the ping. Yeah, when I read "randomly picked" I immediately thought of this kind of attribute setting. :)
So... I'm not bullish on this! What matters most is that nor undo nor dirtiness break due to this attribute. If __unstableMarkNextChangeAsNotPersistent achieves this in practice, I wouldn't block it. But I do agree with @Mamaduka on the principles (avoiding fragile side effects, teaching bad patterns).
There was a problem hiding this comment.
Yep, you'll want to follow up in #79212 which I'm grateful to George for making!




What?
Currently when you insert an Icon block, it shows gray placeholder material:
This is confusing as far as next steps, and not necessarily that useful.
This PR does two things, in two commits. With only the first commit, it removes all placeholder information and simply displays the
infoicon by default:It's a simpler user experience, you immediately get an icon, and we can pick which icon is the default. For now I picked info, but it can be any of the approved icons.
It also forces you to learn the user experince: you do see an icon, but if you want to change it, you have to click the "Replace" button.
The second commit, which we can keep or drop, upgrades this step with randomisation. A random icon is picked as soon as you insert the icon block:
This is a fun little addition that shows some of the diversity of the block. It does come with the downside of a very brief flash of no icon upon insertion, blink or you'll miss it.
Why?
The placeholder added some complexity in code and user experience that we could arguably do without.
Testing Instructions
Insert the icon block, observe that it picks a random icon. Observe that if you then manually replace it with something else, you can still undo back to the one it initially started with.
Use of AI Tools
Paired up with Claude Opus 4.7.