Add site logo crop #31607
Add site logo crop #31607
Conversation
|
This is going to be so extremely useful for the site logo block, thanks for working on it! (Oh and by the way, I feel like at this point you've deserved to be admitted as a contributor, if you'd like to work on the main branch rather than your fork
In general this seems to be working well. There are some flow aspects to the cropping system that could be refined to benefit both this and the Image block, but we can look at that separately. As shown at the end of the GIF, if I change the aspect ratio of the Site Logo the general shape/silhouette of the overall block does not appear to be updated. I wonder if that's related to #29217? |
It seems like it might be. The width calculation seems to be a render behind which causes issues both here and when the resize happens. |
|
@jasmussen I finally circled back around on this, and I think the issue is fixed now after the rebase. Give it another look, and if everything is good we can get this merged :) EDIT: I was able to reproduce it again now. Looking into the fix. |
|
I was just about to compile this, then saw the edit :D Please do ping me as soon as you can use my help, whether as a review or otherwise. |
Very nice, thank you!
If you can get a sanity check on the code it would be nice, but functionally this is solid!
ajlende
force-pushed
the
add/site-logo-crop
branch
2 times, most recently
from
eb81d5d
to
bab8025
|
Is this one blocked? |
Conflicts: package-lock.json packages/block-editor/README.md packages/block-editor/package.json packages/block-library/src/site-logo/edit.js
|
There was a bug with the applied image not showing up in trunk for a while, but it seems to be resolved. I merged in the changes and it seems to be working as intended again. |
| size={ { | ||
| width: currentWidth, | ||
| height: currentHeight, | ||
| } } |
Using currentWidth and currentHeight fixes the bug described in #29217
When cropping, if we deselect the image block the crop tool stays on. This creates a couple of issues;
- At first it's hard to tell that the crop hasn't been applied, even after updating the post. I think it might be good to either Apply or Cancel when deselecting the block, or Applying when the post is saved while cropping.
- It becomes hard to select the image block again; dragging seems to interfere with clicking on the block. A way I found to achieve it was to tab into the block, and then clicking it.
I'm also wondering if it is possible to allow to "decrop" the image; as of right now, Applying the crop creates a new one, and in order to un-do from this point it's necessary to replace the cropped image with the original one.
Nice catch! Deselecting should match the behavior of the image block now.
Seems like a good idea—we could update the cropper to make sure an undo layer is created when hitting apply so we can, at least, hit ctrl+z to undo it until a page refresh. Right now, this matches the behavior of the image block, so it's probably best left as a follow-up to fix in both places. |
Right now, this matches the behavior of the image block, so it's probably best left as a follow-up to fix in both places.
Agreed! LGTM :)
Description
Part of #30406.
Also fixes #29217 in 93dc967. This can be moved to another PR if people would prefer to keep the fix separate.
Adds cropping to the site logo by:
__experimentalcomponentHow has this been tested?
Screenshots
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).The text was updated successfully, but these errors were encountered: