Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix resizing items to top and left with GridItemResizer #60986
Fix resizing items to top and left with GridItemResizer #60986
Changes from 8 commits
c0d99f0
333eefc
f434924
0a6f97b
84f1cb7
7e4dbb8
19e8ae2
6957c76
4bc0167
e026dde
69368e4
795d661
20315d6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What do you think about doing this with a CSS classname instead of a
styles
object?BlockPopoverCover
could have a.block-popover-cover
and.block-popover-cover__cover
class and.block-editor-grid-item-resizer
could have.is-right-justified
and.is-top-aligned
variations.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.
I mean we could do that but is there any advantage to it? Using
styles
is more consistent with how the component works, like with thewidth
andheight
props.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.
It lets us avoid adding the new
additionalStyles
prop. Not a big deal sinceBlockPopoverCover
is not a public component but nice to keep API surface area low where possible. Up to you.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.
Hmm yeah. I think the additional prop is a worthwhile trade-off for better legibility. Also the inline style means it's less likely some over-specific CSS will break this at any point 😄
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.
Should this fix live in
ResizableBox
so that future users of the component don't run into the same issue?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.
Hmm good question. This issue only surfaces when we set
bounds
on the box, so it's not really useful for all the instances. And this fix depends on knowing about a parent element, whichResizableBox
doesn't. It doesn't even implement its ownonResizeStart
, just passes it directly toResizable
. So I'm not sure it's worth adding complexity to the component for what so far is a pretty niche use case. Maybe worth doing it if similar instances pop up in other 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.
The
mouseup
handler could be added toownerWindow
orownerDocument
to avoid it having to know about a parent element.But yeah, not sure if it's needed in
ResizableBox
, up to you.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.
Hmmm not sure that'll work if we have the iframe in the way. The parent element I added the event listener to is in the actual editor canvas, but the component isn't.
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.
We need to remove this event listener somewhere to avoid memory leaks.