Try: Site Logo placeholder tweaks #35397
Conversation
jasmussen
added
[Status] In Progress
|
Size Change: +1.16 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Nice, this feels pretty close to having all needed functionality. One thing I noticed, was that we need a different area to show any error messages. The placeholder here will be too small: Edit: okay this is a problem in trunk too :D
After: using snackbarCleanShot.2021-10-06.at.13.47.07.mp4 |
|
I think this is our first usage of the non-scaling-stroke vector effect. I'm noting the browser support here. We dropped IE support, so I think we should be okay here.
|
|
@jasmussen I pushed up changes to add the upload icon, hover tooltip, moved the error notice to the snackbar, and provided some passthrough options to the MediaPlaceholder. The tooltip interaction doesn't feel quite right to me, so I suspect that can use some further work/thinking. Feel free to modify anything that you think needs some further changes. 👍 CleanShot.2021-10-06.at.13.56.32.mp4 |
|
Thanks so much for doing all this work! It's a master class for me. I agree on the tooltip, but thanks for adding it. I'm going to try today and shift things in this direction:
That way, the tooltip can just be a tooltip that happens as usual when you hover the upload button, rather than the placeholder itself. |
|
Such nice work, thanks so much for all your help. I managed to push a few tweaks getting it to this state:
I feel like the behavior is now pretty solid. I'm going to rebase to fix a merge conflict, fix a couple of small bugs I discovered with resizing, and then mark this one as ready for review. |
jasmussen
force-pushed
the
try/site-logo-tweaks
branch
from
513aa52 to
f081e71
Compare
|
I rebased, the branch, fixed an issue with an inherited border radius, and added some placeholder sizing smarts: At the moment, you can resize from the default 120px dimensions by adding an image, resizing, then removing the image again, as shown in the GIF above. It's a bit of a roundabout way, but the placeholder is now smart enough to account for that:
Separate to this PR it might be nice to introduce the resize component to the placeholder as well, so that it can just be sized right there. All of this is also to say: this now feels solid to me, and I'm marking this one ready for review. |
gwwar
added
[Type] Enhancement
| const blockProps = useBlockProps( { | ||
| ref, | ||
| className: classes, | ||
| } ); | ||
|
|
||
| const label = __( 'Add a site logo' ); |
There was a problem hiding this comment.
Did we want to give a hint about drag&drop or clicking for the media library?
There was a problem hiding this comment.
Per a newer comment on the ticket, I thought it worth it to start small. But it reminds me, I forgot to probably test the drag and drop 🙈 I need to give that a quick spin. It's probably fine.
There was a problem hiding this comment.
Thanks so much again for all the help. I took the drag and drop for a spin and pushed a tiny tweak, otherwise it's looking good!
There was a problem hiding this comment.
Nice updates @jasmussen This tests nicely for me! I left a note about the tooltip copy, but it's not blocking. I agree on adding a follow up PR to allow resizing without an image set, it feels pretty natural to have that with a placeholder wireframe available.
We'll want a +1 review for this PR since I also pushed changes here.
One thing for folks to look over are the new mediaLibraryButton and placeholder component passthrough props that I added to the block-editor/media-placeholder. Site Logo needed some more drastic component changes, but we can perhaps later refactor this into it's own component and pull out some of the upload checks into hooks.
|
I pushed a small fix to a blinking behavior that happened when you drag/dropped an image onto the icon itself, so now it works as intended:
That fix is also part of this separate PR that overhauls the drag and drop animations. |
jasmussen
force-pushed
the
try/site-logo-tweaks
branch
from
19dc5e9 to
166a766
Compare
There was a problem hiding this comment.
Approving this since I think enhancement looks good, and follow-up PRs can address my feedback.
I noticed that if I change the size and then reset the Site Logo, the placeholder shrinks:
CleanShot.2021-10-08.at.13.15.43.mp4
New placeholder prop: I think it can accept a component instead of a callback. Similar to how the InnerBlocks appender works. We're already passing all the props we might need to MediaPlaceholder.
We also need to document these two new props.
|
I ran into the same size issue, and I think the solution is to allow the resize handles even in the setup state. The thing is, this particular block comes with a default size (120x120), which is applied if the block isn't manually resized. As soon as the block is manually resized, no min-width/heights are applied, which in the setup state means it collapses to the smallest button size. However it makes sense for themes to be able to predefine dimensions of the site logo block, to fit theme metrics, and this would further be enhanced by being reflected in the setup state. Thank you for the review! I'm going to land this one for now! |
Description
Fixes #35096. This improves the setup state of the Site Logo in a few ways. Here's what it looks like in trunk, square and basic:
Here's what the PR does:
At the moment, you can resize from the default 120px dimensions by adding an image, resizing, then removing the image again, as shown in the GIF above. It's a bit of a roundabout way, but the placeholder is now smart enough to account for that:
How has this been tested?
Insert a site logo, whether on its own or inside a navigation block. Test resizing it, test setting the rounded style, try adding a logo in both styles.
Checklist:
*.native.jsfiles for terms that need renaming or removal).