-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Try stickier block toolbars in Customize Widgets #32490
Conversation
Size Change: +2.1 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
@stokesman This is quite a big change (in scope and lines of code), especially for consideration as part of WordPress 5.8. I started looking at it but found it hard to review (and I realise it's based on another PR). Would be good to see some more justification for the decisions taken and what alternatives were considered in the process of developing this. Also would be great to see some more comments throughout the code or attempts to make the code more self-documenting. I found the fact that popover now has lots of different The |
Thanks for the feedback @talldan. I did intend to straighten it up before marking as ready for review and your comments will aid that effort.
It'd be possible to reduce flexibility a little and have fewer props.
👍 |
02678d0
to
9e1f4a2
Compare
packages/customize-widgets/src/components/sidebar-block-editor/index.js
Outdated
Show resolved
Hide resolved
9e1f4a2
to
7f32e0b
Compare
@talldan, I've rebased and reduced changes to what's relevant. I also reduced the added props to more tightly fit the present use case and added comments. I think before anyone puts in the effort of a full code review the UX still needs vetted. So I'd like to bring up what has so far been the concern regarding this approach, mentioned by @kevin940726 and @critterverse
I've given that some thought but wager it would seldom cause confusion. I suppose a trial of it would be the only way to learn. We could also look at ways to indicate the selection is out-of-view. One quick idea: Toward the end of the screen recording in the description, I demonstrated dragging to reorder a block that was out of view. That was a nod toward looking out for such side effects. That particular one seems like it could be considered a feature by allowing an easier reordering task in some circumstances. |
@stokesman What do you think about moving the customizer title sticky changes into a separate PR? That part seems to add quite a bit of complexity and the performance needs improvement. I also think an option could be a patch to WordPress core for changing how it works. If the PR just focuses on the block editor top bar and toolbar, that should hopefully make it a bit easier to iterate. |
BTW, I also had some code for reveal on scroll in this branch - trunk...try/customize-widget-revealing-topbar That was an experiment to try the behavior on the editor top bar. An approach I took there was to only change edit - actually looking at the code I seem to be misremembering, it seems to use the delta too. The performance difference might be more related to updating the React |
A fine idea 👍 I'd been starting to think it myself. Certainly even if we find a performant solution it'd be intended as a crutch until core could be updated. |
7f32e0b
to
d9c59f7
Compare
Removed the reimplementation of the Customizer’s sticky title. Number of lines changed greatly reduced 🎉 . |
This is looking good with the most recent change to the Customizer title area 👍
Explored a couple of options for this and I think maybe the best option is potentially the most straightforward — dimming the toolbar after the selected block has scrolled out of view. The borders and icons are currently toolbar-dimmed.movEdit: Updating to include another exploration here as well since it might be helpful to get more opinions on this. In this version, the toolbar would anchor to the block as usual until it scrolls out of view, then collapse into a single-item toolbar (like a parent selector). toolbar-collapsed.mov |
I wonder if this could work 🤔? |
@critterverse, out of those explorations I tend to think the collapsing one would be the better of the two though probably a bit more effort. I'd also like to think we could allow the drag handle to still be available. Or maybe even a separate "scroll into view" button replace the ones that are hidden. I've been thinking on it and coming around to prefer, at least for now, something like what @kevin940726 has suggested. I'd still like to see it done with the stickier toolbar as in this PR, so there's no overlap even while it transitions in and out. |
- Add props to BlockTools and Popover to support expanded sticky area and sticky position off the bottom - Add logic for sticking the popover as its anchorRect overflows the bottom of its boundary - Remove now extraneous z-index rules and map entry
This sounds like a good plan but I'm having trouble picturing how the overlap can be avoided while fading — the toolbar and top bars are still going to intersect at some point before the toolbar can fade out, no? Also added one more previously explored idea in #32061 where the block toolbar could go in front of the header bars rather than behind (like is happening currently with the footer controls at the bottom of the Customizer sidebar). I'm open to whichever approach we can get working better with the sticky editor bar, which is a nice usability improvement over what we have now! |
d9c59f7
to
8352118
Compare
I've pushed an update to hide the block toolbars when their block is out of view. Instead of a fade, a transform is used since it was fun and may help reinforce why the toolbar is hiding. I'm not attached to that and fine with changing it. To try the fade while testing this PR the following can be pasted in to the styles in dev tools: .components-popover.block-editor-block-list__block-popover .components-popover__content[data-away]{
transition: opacity 0.1s linear;
}
.components-popover.block-editor-block-list__block-popover .components-popover__content[data-away].is-away {
transform: none;
opacity:0;
} The screen recording in the description is updated.
That is definitely an improvement over trunk and happens to be where this PR started. I've put the first two commits here into a separate PR #32650. |
Awesome, this was so helpful to be able to try out several different approaches at once!
I really like how the version with the fade is working — props @stokesman and @kevin940726 for this elegant solution, it came together nicely with the sticky editor bar 🎉 |
Thanks for testing it out @critterverse 🙇 . I've pushed an update to switch to the fade transition and that allowed a little simplification. I've also simplified the logic determining if the selection is in view or not and that should ease code review. Which I think could be the next step here. Marking it ready. |
Thanks everyone for working on this. It's looking really good, but let's punt it to WordPress 5.8.1 as there's a few more rounds of UX/dev feedback required and we've only a limited amount of person hours available before the 5.8 code freeze on June 29. |
An experiment in how to work out the recent toolbar overlapping issues in the Customize Widgets editor. The two high-level changes:
UPDATE: The floating block toolbar now hides when its block goes out of view (its position is still stuck to the scrolling boundaries to avoid overlap during transition).
How has this been tested?
Manually…
Screenshots
customize-widgets-moar-sticky-4.mp4
For posterity, the version with alternate transform transition of the toolbar
customize-widgets-moar-sticky-3.mp4
Types of changes
Checklist:
*.native.js
files for terms that need renaming or removal).