Update frame resizing #49910
Update frame resizing #49910
Conversation
github-actions
bot
added
the
First-time Contributor
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @SaxonF! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
Size Change: +22.1 kB (+2%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
mtias
force-pushed
the
add/edit-site-frame-resizer
branch
from
d0b2752 to
d51b541
Compare
|
Flaky tests detected in a2bf020. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5027015473
|
|
Pushed a couple changes:
|
mtias
added
[Feature] Site Editor
|
Took this for a spin. It's mighty impressive from a quick test, here scaling down, up, and into fullscreen:
If you release the frame when it's small though, then grab and size up again — or perhaps if you linger with the resize handle near the vertical edge of the left menu — something appears to go out of sync and becomes disconnected:
Not sure if that helps, it seems those snap calculations are tricky. Edit: One more thing that's only adjacent to this PR but more noticable: when you move the cursor over the frame, it scales up a little bit. If you then move the cursor to the resize handle and start resizing, it remains in its slightly scaled up state, which is okay, but it feels imprecise somehow. |
There was a problem hiding this comment.
I took a look at this PR — this part of the codebase is basically new to me. If I understand correctly: previously to this PR, the "resizable" part of the UI was the sidebar. With this PR, the canvas becomes the resizable part.
If you release the frame when it's small though, then grab and size up again — or perhaps if you linger with the resize handle near the vertical edge of the left menu — something appears to go out of sync and becomes disconnected:
This is weird. I took a look, and could initially reproduce. Then I made a few tweaks and wasn't able to reproduce the error anymore. I removed my tweaks and I continue not to be able to reproduce the problem. FYI, I was disabling the hover scale-up animation and anything applied when the isOversized internal state is true. @mirka , maybe this is something that you could look into?
Apart from the inline comments, here are a few observations after taking a look at the code and playing around in the browser:
- The "scale up" animation when hovering the canvas doesn't feel very snappy, and causes some visual glitches: the canvas becomes slightly blurry (probably because of sub-pixel rendering), and it can easily enter a "jittery" mode in which the scrollbar are quickly shown and then hidden in rapid succession.
- Overall, there's a lot going on, which makes this PR quite hard to debug IMO. I would consider working in isolation, separating the "resizable layout" part from the "animation" part.
- I would also consider creating a simpler version of this in a codesandbox, in order to be able to focus on this specific task.
Other few comments not necessarily related to this PR, but more in general about this section of Gutenberg:
- I would reconsider part of the DOM structure. In particular, I would look into simplifying the Hub and the sidebar Header, leaving the top left logo separated from the "Gutenberg" text.
- I noticed something similar to a caret blinking inside the editor canvas, in the top left corner
|
|
||
| setIsResizing( true ); | ||
|
|
||
| if ( event.clientX < 344 ) { |
There was a problem hiding this comment.
Relying on event.clientX isn't probably the most robust solution here, since the pointer position may not be always accurate (for example, what happens if the pointer leaves the browser window while dragging?).
If we want to know the gap between the canvas and the left edge of the viewport, maybe we could subtract the canvas' width from the viewport's width?
There was a problem hiding this comment.
Noting for posterity: clientX does in fact give us usable numbers even when outside the window (e.g. negative numbers for the left edge).
However, this PR now relies solely on the frame width and the ownerDocument width to determine the isOversized boolean. It's just a tiny bit more generic than relying on clientX, but no strong opinion here. I could just as well revert to clientX, since it's simple and this ResizableFrame component is not really generic/reusable anyway.
There was a problem hiding this comment.
I think I like the current version better, we can keep as-is
|
|
||
| return ( | ||
| <ResizableBox | ||
| as={ motion.div } |
There was a problem hiding this comment.
I would try to avoid mixing motion and ResizableBox via polymorphism (as prop).
More in general, given that we're still trying to figure out the best UX for the resizing part, I would almost suggest that for now we remove all animations and focus on ResizableBox.
Once we feel like we nailed the resizing functionality, we can add animation back and fine-tune that too. For example, specifically to this component, I think that all animation-related functionality could be added to the .edit-site-the-frame__content component, instead of using the as prop.
There was a problem hiding this comment.
I would try to avoid mixing
motionandResizableBoxvia polymorphism (asprop).
I realized that, in the current implementation, it is unfortunately necessary that the ResizableBox is also a motion.div 😬 In re-resizable, controlled size prop changes aren't reflected at all until resizeStop, meaning that we cannot do controlled size changes during a resize gesture. Our controlled height changes are actually being executed through the motion.div's height, not re-resizable's size 😓
There is a possibility we could move the height management to re-resizable by giving it dynamic lockAspectRatio values (similar to what we do with resizeRatio). But since it is functional right now, I recommend deferring that to a nice-to-have follow-up.
There was a problem hiding this comment.
Sounds good, will leave this convo open for posterity
There was a problem hiding this comment.
If you release the frame when it's small though, then grab and size up again — or perhaps if you linger with the resize handle near the vertical edge of the left menu — something appears to go out of sync and becomes disconnected:
I also had trouble reproducing this, but I finally got it to happen once. I'll look into this a bit more tomorrow.
|
@mirka @ciampo I'd recommend also checking out |
|
@mirka it's easy to reproduce — if you resize the frame below 100% the initial width is recomputed, so when we do the flexbox change (above 100%) the width shifts back to the initial conditions. Right now it's being force to be at least 100% mind width, which is why there's a threshold in which the mouse position needs to catch up. If you try removing the CSS rule of |
| const handleResize = ( event, direction, ref ) => { | ||
| const updatedWidth = ref.offsetWidth; | ||
| const updatedHeight = ref.offsetHeight; | ||
| const handleResize = ( _event, _direction, _ref, delta ) => { |
There was a problem hiding this comment.
The split across separate functions makes the logic much more clear:
- initial
useEffectto calculate the "first-render" width and aspect ratio - when the resizing starts, get the starting resize width and set
isResizingtotrue - the
onResizecallback only takes care of updating the resizable box size, and settingisOversized - when the resizing stops, check if we need to snap back to initial position or to switch to edit mode, and reset all the flags appropriately.
|
Behavior-wise, I think we are now at a shippable place, with the resize ratio working as expected and some cursor glitches addressed. Remaining tasks:
|
|
Thanks for all the work here. It's looking good. The only issues I've run into have been after resizing the window which seems to throw off calculations. I think this could be a follow up task though as I see this branch as a big improvement over what's live already. |
|
I'm giving this PR a quick look. I'll rebase on top of trunk, solve conflicts, and force push a new version soon. |
ciampo
force-pushed
the
add/edit-site-frame-resizer
branch
from
28ba8d7 to
785c6b2
Compare
There was a problem hiding this comment.
This is definitely in a much better state.
I noticed a couple of scenarios that need polishing:
- When snapping the resizable frame back to the sidebar or switching to the canvas mode, I can see that the resizable frame doesn't animate smoothly from its current size, but it instead seems to "jump"
frame-resize-glitch.mp4
- As @SaxonF also mentioned, the resize logic seems to get a bit out of sync after resizing the browser window
frame-resize-glitch-2.mp4
We can probably merge this PR as is (although it'd be great to get another pair of eyes to confirm that everything looks ok), and follow-up with improvements:
- around the glitchiness described above
- clean up the code further and look into avoiding mixing
motionandResizableBox(see this comment)
Borrowed from my initial review:
- I would reconsider part of the DOM structure. In particular, I would look into simplifying the Hub and the sidebar Header, leaving the top left logo separated from the "Gutenberg" text.
- I noticed something similar to a caret blinking inside the editor canvas, in the top left corner
|
Just realized that there's a potentially related e2e failure, that will need more investigating |
It should gradually reduce from original aspect ratio until reaching a 9 / 19.5 view.
Reset the initial aspect ratio if the frame is resized slightly, and trigger full screen if the frame is resized far enough over the sidebar.
ciampo
force-pushed
the
add/edit-site-frame-resizer
branch
from
785c6b2 to
44828f7
Compare
|
I'm struggling to debug tests — on my local machine, tests from the I've rebased again, let's see if e2e tests keep failing. |
@ciampo, I've pushed some fixes - a couple of selectors needed to be more specific as they were hitting more than one element to be clicked. There might be one legit failure, though, within the template part spec. I'll double-check when the current job is done. |
There was a problem hiding this comment.
This is working much better!
I noted down a few refinements to consider, but there are not blockers.
- It might be something wrong with the larp function, but the heigh of the frame changes slightly after it reaches full height and you keep going over the sidebar. It's very small.
- We should move the subtle scale effect on hover to the actual frame. Right now on smaller sizes it's weird since hovering on the dark material triggers the zoom.
The `.getByRole()` way resolves a bit too early.
|
Thanks for the great collab here, everyone! I addressed the remaining e2e failures. I'll merge now to avoid further conflicts in the layout file 👍 |
| // A role selector cannot be used here because it needs to check that the `is-busy` class is not present. | ||
| await this.page | ||
| .locator( '[aria-label="Editor top bar"] [aria-label="Saved"].is-busy' ) |
There was a problem hiding this comment.
Nice find. I think it would be good to address this on an implementation level, though (not a scope of this PR). The button should probably say Saving, not Saved as long as the request is pending (.is-busy ATM). cc @kevin940726
There was a problem hiding this comment.
Yes, I totally forgot to TODO comment this. Thanks!
|
Jotting down a few more follow-ups I noticed while fixing #50836.
|
|
Hello folks, this PR introduce a regression to the "resizing handle" see #51267 Basically, the handle should stay visible and be a focusable element even if we're not actually hovering the iframe. |
|
The handle can be hidden if it's not hovered or focused but it should be in the DOM. |
| canvasWidth = canvasSize.width - canvasPadding; | ||
| } | ||
| const [ fullResizer ] = useResizeObserver(); | ||
| const [ isResizing ] = useState( false ); |
There was a problem hiding this comment.
isResizing is always false after this PR. I understand this is a leftover that can be removed. I've prepared a PR to do so at #57119
Updates to the frame: