-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
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 😉). This is what I see: 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. |
fb356e0
to
8e19d32
Compare
@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. |
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.
eb81d5d
to
bab8025
Compare
bab8025
to
e410a4d
Compare
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, | ||
} } |
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.
Using currentWidth
and currentHeight
fixes the bug described in #29217
3535708
to
2574192
Compare
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.
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. |
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.
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 93dc9675cb448586621a840a310202fe3acdee65. This can be moved to another PR if people would prefer to keep the fix separate.
Adds cropping to the site logo by:
__experimental
componentHow has this been tested?
Screenshots
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).