Split content view with open metaboxes #64351
Conversation
stokesman
added
[Type] Bug
|
Size Change: +2.03 kB (+0.11%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This PR is working pretty well. My only concern is that the meta box height will be fixed at 50%, which might make it difficult to see the meta box content. I'll ping @earnjam and @arnaudbroes about this:
Also, the E2E tests related to meta boxes are failing, so we may be missing some problems. |
|
@stokesman @t-hamano How about using ResizeableBox to make the height changeable? |
@torounit, I did think it would be likely to be suggested. It’s rad that you made a working demo. I'm not sure if it’d be necessary for a first try with this approach but I’m not opposed to it either. With or without it, I believe the priority would be determining and addressing any responsive issues. Like maybe at less than a certain height the view should not be split and work a bit more like #64247.
Hopefully the latest commit takes care of any e2e failures 🤞. |
|
I'm not sure about this. It's a solution that imo makes both experiences a bit worse. You no longer have a full view of the content, and neither do you have a full view for the meta boxes. What happens when you have lots of collapsed meta boxes? The height of all collapsed meta boxes will need to be capped. |
|
What we could do is combine both approaches. So have a toggle in the header (the one from Move legacy meta boxes to toggle), and also the resizing. So by default it would put content at 50% and meta at 50%, but you have the option to hide the meta boxes with the header toggle, and the option to resize the height (maybe even all the way to 100%). |
|
Currently, the header component is integrated into the |
It can probably be an option? @stokesman would you be ok in adding it to your PR here? |
|
If this PR is updated, I'd be happy to test it. |
I find the general idea favorable though lean toward using the details element instead of a toggle in the header for a few reasons:
I’ve tried it and it seems workable. I will probably push an update with that soon but am open to hearing any thoughts on why a toggle in the header would be better in case I'm missing something. |
|
I've updated the PR in various ways. The primary changes:
Hopefully the decisions will be self-explanatory with some testing but I'm happy to discuss and open to change them. EDIT: I’ve updated the screen recording in the description in case anyone wants a quick look. |
|
I’d like to ask @ppolo99, @hrkhal and @aaronjorbin to have a look at this as they did provide feedback on #64247 and may find some of their concerns addressed here. |
| max-height: 50% !important; | ||
| } | ||
|
|
||
| & .components-resizable-box__handle-top { |
There was a problem hiding this comment.
Rather than overwriting the existing handle with CSS, as implemented here, how about defining a custom handle?
If we use a button as the handle and add a keyDown event, we should be able to focus the resize handle with the keyboard, making it operable.
There was a problem hiding this comment.
I’d thought this too and even think it’d be necessary but hoped to spare some effort while uncertain if this PR will garner favor.
There was a problem hiding this comment.
Now that the PR is approved, would it be a good time to apply @t-hamano 's advice?
There was a problem hiding this comment.
I’ve got a branch that does it. It’s looking like about 160 insertions(+), 40 deletions(-) minimum from this branch so not too trivial. There are also design/UX considerations so a clean thread would be nice for any discussion. I’m thinking a separate PR. We can either merge to this one or to trunk if we land this before then.
There was a problem hiding this comment.
Sounds good — feel free to ping on that separate PR
|
Since #64247 has been added to the 6.7 Editor Tasks board, I will also add this PR to that board. |
|
This is wonderful! Can we keep the details toggle for any screen size? I think we need a way to quickly show and hide at all times. Not sure if this is already done, but it'd be great to persist the open/close state. With those changes it would be good to merge imo!
|
If considered necessary, I think it’d be nice to avoid the UI bulk of a resize handle stacked atop the
I take it you mean persist it in user preferences. That’s not done and I agree it’d be nice. Perhaps the resized height should also be persisted? |
I think a toggle is necessary, imo. Like I said before, this can be in the form of a top toolbar button. I think that's actually what @mtias preferred last time we talked about it. |
|
You can actually keep everything as it is now, and just add the toggle to the top toolbar to add/remove the whole thing. The details is probably still useful for smaller screens. I do also slightly prefer a bottom toggle compared to a header toolbar toggle, but if it's technically too difficult it's better to have something and revise it later. |
stokesman
force-pushed
the
try/metaboxes-flex
branch
from
557f505
to
c31b7cf
Compare
|
I’m eager to land this but did come across a couple more issues I think are suited to solving here so I’ve pushed a couple commits. 1. Content in meta boxes can appear in front of the resize handle or the details summary
🛠️ SOLUTION: 3da7979 Puts 2. Compatibility for non-iframed conditionsWhen the editor canvas is not iframed the UI changes here don’t work well. These conditions are becoming more rare but for now still need to be accounted for. non-iframe-resize-funk.mp4🛠️ SOLUTION: c31b7cf Shows the new UI only if the canvas is iframed. |
Lead dev for AIOSEO here. I've been keeping an eye on this thread. I'll go ahead and already create an issue in our development repository for this. Do you have any idea what Gutenberg plugin version we can expect to see these changes in? That way I can make sure to test this properly. |
| // This is overriden by an !important rule that applies until user resizes. | ||
| maxHeight="100%" |
There was a problem hiding this comment.
Nit: The same style is defined here, so is this prop unnecessary?
There was a problem hiding this comment.
Actually, a brief retest, neither the prop nor the rule in CSS seem necessary 😅. The branch I have that adds keyboard support uses this prop with specified pixel value (for reasons which which I’ll elaborate in its PR) and I’ll leave this be here. On the other maybe we can remember to check if the CSS rule has any purpose.
|
Another important thing I noticed is that if I reload my browser after moving a meta box to the sidebar, the button to move the meta box is no longer there.
My guess is that this button is controlled by jQuery and checks whether the metabox area element itself exists. But in this PR, if the metabox doesn't exist in the Perhaps the diff --git a/packages/edit-post/src/components/layout/index.js b/packages/edit-post/src/components/layout/index.js
index 425b7dfb95..2cbf5c32e8 100644
--- a/packages/edit-post/src/components/layout/index.js
+++ b/packages/edit-post/src/components/layout/index.js
@@ -168,7 +168,8 @@ function MetaBoxesMain( { isLegacy } ) {
get( 'core/edit-post', 'metaBoxesMainIsOpen' ),
get( 'core/edit-post', 'metaBoxesMainOpenHeight' ),
isMetaBoxLocationVisible( 'normal' ) ||
- isMetaBoxLocationVisible( 'advanced' ),
+ isMetaBoxLocationVisible( 'advanced' ) ||
+ isMetaBoxLocationVisible( 'side' ),
];
}, [] );
const { set: setPreference } = useDispatch( preferencesStore );This way, the resizable box will be rendered even if the metabox doesn't exist in the main area, but users who are bothered by that can lower the height to zero via the handle.
|
Good catch! For now it does look like we just have to accept the downside and I’ve applied your suggestion in 8df9313. To do better than that it would probably require a WP core patch or some |
|
@stokesman Thanks for the update! I think we're ready to ship this PR, what do you think? |
@arnaudbroes, 19.3 👍 |
|
I'm not sure that using the |
|
Hey @stokesman / @ellatrix 👋 Would you be able to help write a Dev Note for this feature? We are tracking all the dev notes for 6.7 them in this tracking issue: #65784 We are hoping to have all drafts compiled by October 13th so we have some time to review before RC1 |
stokesman
changed the title
|
Is there a way to disable this split view? I have some custom css/js which creates tabs from acf groups and dont need that extra space. |
At the moment, if you register a block whose Alternatively, something you could do is change the height of the meta boxes pane. Like so: wp.data.dispatch('core/preferences').set(
'core/edit-post',
'metaBoxesMainOpenHeight',
400
);Note The height is intended as a user preference and should not be modified in most cases. However, in case it’s not already set, it may be okay. The current height can be read with: wp.data.select('core/preferences').get('core/edit-post', 'metaBoxesMainOpenHeight');It should be |
What?
An alternative to #64247 to have the iframed canvas work with metaboxes in the Post editor.
Why?
Some folks want to have both metaboxes and canvas content visible at the same time.
How?
flexto split the content view and have both areas scrollable without either being nested in the others overflow.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
metaboxes-flex-moar.mp4