-
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
Add a built-in exploded mode instead of a separate view #40376
Conversation
Size Change: +1.3 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
Something is off with my local environment and I can't test the site editor 😩 I'm trying to figure out what's up. But from what I can tell by the CSS, this should definitely make animation easier. Thank you for looking into it — if we do go this route it would be good to think long about how we can best manage the hacky pieces, see whether the tradeoffs are manageable or worth it. |
@jasmussen Try cleaning local storage, maybe the leftovers from the other PR are breaking the site editor in this one. |
That did the trick, many thanks! Testing this branch I can only validate that this feels massively snappier, and I can easily see how animation can play a part here. All the more validation of my thought before: whatever we can do to identify the pieces about this approach that make us uncomfortable and seeing how/if we can address them would be great. |
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'm surprised that this simple approach is working so well. It also has the advantage of being in the block editor so we can easily include in edit posts for template editing.
I guess it would be good to include in this PR the mechanism for when a click happens on an exploded mode to go back to normal edit mode with the block that was clicked on selected.
And it would also be good to fix the in between inserter
But other than that I think it is an approach we can try 👍
@@ -160,10 +162,70 @@ async function loadScript( head, { id, src } ) { | |||
} ); | |||
} | |||
|
|||
function useExplodedModeBackgroundStyles( isExplodedMode, deps = [] ) { |
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.
This hook seems to be the most hacky part but it is working 👍
Made some progress here. Would love some 👀 on this. My feeling right now is that we have a lot of "modes" and a lot of code that does similar things but differently (outline mode, exploded mode, navigation mode, active entities (something for template parts), block overlay (something used in template parts and reusable blocks)). We keep adding modes and experiments without cleaning what we already have which is basically the perfect recipe for bugs. So while I think the approach in this PR could work, it does add more complexity and have a way higher potential for bugs and performance impact. |
ee234ef
to
c9f96f8
Compare
This is looking better and better, the animation is a great touch. I'd love for the spacing to also animate — right now it's a fast shift from normal to exploded view. Not necessarily something that has to happen in this same PR if you're seeking out a small first version, but something to look at. We should also use the same dark background as shown in @jameskoster's prototype in #40319. Speaking of "modes", yes I agree, there's a great deal of cleanup work to be done here. |
In the three last commits I had to do some bigger refactoring to the BlockPopover components, in order to be able to render the "in between" inserters at the right position. You'll notice that when scrolling, the position of the inserter might take a few milliseconds to adjust itself. This is basically one of the downsides of the approach in this PR. Any UI we render that is contextual to blocks has to be rendered outside of the canvas (iframe), meaning positioning it properly is challenging (has to use popover). Potentially we could do some small tweaks, like hiding the inserters when we're scrolling or things like that. Let me know what you think |
// This is a temporary hack to get the inbetween inserter to recompute properly. | ||
const [ positionRecompute, forceRecompute ] = useState( {} ); | ||
useEffect( () => { | ||
const intervalHandle = setInterval( forceRecompute, 500 ); | ||
return () => clearInterval( intervalHandle ); | ||
}, [] ); |
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.
This is a hack to be able to adjust the popover position while the animation is on going. Basically the popover position is dependent on the "block position" and every time the block position changes, we need to recompute the popover position and unfortunately we don't have APIs to "subscribe" to position and size of a given DOM element. (without inserting anything inside that element like we do with the resize handler).
Please let me know if there's anything better that we can use here.
Yes, that does feel a bit gnarly, and if we can't make these feel more resilient in this approach we might want to do something like that. That said, the animation and instantaneous behavior of clicking the button does feel very strong in this one. |
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'm not able to use the section movers with success. Sometimes I click and the section disappears other times checking does nothing like if the click did not happen. |
If I select the footer, sometimes clicking on the header does not select the header. |
If you select the header even in normal view, you'll see that you're selecting more than the "black" background. There's a spacer at the end of the header template part, So I think that's something in the theme and not a bug. |
This makes sense yes. |
766f695
to
79499f2
Compare
79499f2
to
c95cb08
Compare
Solved some of the issues above, notably: clicking the movers, selection the parent block when entering exploded mode. It still not perfect, I believe there's still an issue that sometimes the "in-between inserters" hide the block (so it appears like a big blank) |
This feels very laggy to me 🤔 Even hover effects have a big delay: laggy.mp4I'm seeing this in and out of Exploded view, but only in the Site Editor. Sometimes child blocks are randomly selected: selection.mp4Some other observations:
|
Yeah, most of the issues/fragility here at the moment is coming from the inserters, how to position them... It's the downside of this approach, I can take more time to polish things more but conceptually, this is the downside of the approach and will be hard to have the same feeling here to the other branch. On the other side, the other branch can't (or let's say 95% certain it can't) have the same animation we have here. -- I'll continue to polish this branch bit by bit during the next days and we'll see how far we can get. |
7af4204
to
8c88927
Compare
This is fixed
I hid the inserters on scroll, it's a bit better but it's not perfect |
This is starting to feel really good! Consequently it's time for a design nitpick – apologies in advance 🙏 Appreciate I may be asking for the moon on a stick, but is there any way to hide the inserters when they are due to be re-positioned? Then we can avoid this jumpiness: jump.mp4Can opening panels trigger the re-position of inserters? At the moment it can lead to mis-alignment: list.mp4I assume the answer to this question will be "no" since we're using On a similar note, the mockups scaled sections down to 70%, lets try that rather than 80%. Is there a way to increase the spacing at the top / bottom of the exploded view? Separate to this issue, but a small visual detail we'll need to address: |
The answer is "I don't know" for both :P. Ideally I'd like to land #40740 before this one and see what impact it will have here and if we can find better ways to handle these cases. |
c0aba72
to
eb2ab13
Compare
a18d694
to
232b6f9
Compare
This was a nice exploration but I'm going to close this PR for now in favor of #41156 There are still too many technical challenges and fragility in trying to "explode" blocks properly. |
Related #39281 #40319
Alternative to #40314
What?
We want to explore an "exploded block view" where the blocks (and sections) of the top level are separated and the actions for each block/sections are separate from the regular toolbar.
See #39281 (comment)
This PR just bootstraps that mode to allow us to iterate on it.
Why?
I think the reasoning is to make it easy to build templates from scratch by leveraging ready to use patterns.
How?
This PR thought differs from the original one in the sense that instead of building this mode into a separate tree view. This PR "just" tries to style the BlockList view. The advantage of this solution is that it's easier to implement the "animation" when entering/existing the mode. The con is that this is basically hacky. We have code to copy the background from the parent (body) to inner blocks dynamically. The other challenge is that it's going to be hard to inject UI: in-between inserter, the proposed right sidebar. As opposed to the other PR I didn't add the "inserter" yet for this reason.
Notes and questions?
I'm surprised that I managed to get this far with this approach. I'm still not sure if it's better though. In terms of code quality, I also don't like how this is very intrusive in the existing UI code... Some bugs/regressions are not to be excluded every time we add exceptions/cases to existing stuff.
Testing Instructions
1- Open the site editor
2- Click the "exploded view" button on the header.