Try 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 Thanks for your opinion. Indeed, we are ultimately aiming to have the post editor always function as an iframe, so it may be something developers have to address anyway. Personally, I think this PR can be shipped as is, but it will have a big impact on many plugin developers, so I think a decision needs to be made on whether to ship this in WP 6.7 or not. Therefore, let me ping @WordPress/gutenberg-core. Summary:
|
| * Internal dependencies | ||
| */ | ||
| import { store as editPostStore } from '../../store'; | ||
|
|
||
| const isGutenbergPlugin = globalThis.IS_GUTENBERG_PLUGIN ? true : false; | ||
|
|
||
| export function useShouldIframe() { |
There was a problem hiding this comment.
In which case the iframe is not used now? Can't we just remove this hook entirely
There was a problem hiding this comment.
Replying to myself: I'm guessing if there are non v3 blocks. I think we should probably reconsider this decision as well. People had time to migrate and we can address this differently now. (Error boundaries for this kind of blocks, or a behavior similar to the classic block). But we should explore this in a separate PR.
There was a problem hiding this comment.
Also WP.com users have been using iframe for all blocks (regardless of version) for block themes for a while. I think that represents a big number of users already, so I'm confident that there are very few blocks that actually break because of the iframe these days.
There was a problem hiding this comment.
If I recall correctly, when the post editor became an iframe in block themes (#46212), plugin developers reported issues (e.g. #47924). So the block API version 3 was introduced and the post editor was changed to not become an iframe, at least when v1/v2 blocks were registered (#48286).
And this PR doesn't just affect blocks: in fact, as mentioned later in this comment, it seems there are still some well-known plugins that don't support the iframe editor.
There was a problem hiding this comment.
And this PR doesn't just affect blocks: in fact, as mentioned later in #64351 (comment), it seems there are still some well-known plugins that don't support the iframe editor.
Do you know if these plugins actually don't support the iframe or just register blocks < 3. My suspicion is that most of the plugins that have blocks < v3 support the iframe but have trouble upgrading for other reasons.
There was a problem hiding this comment.
Do you know if these plugins actually don't support the iframe or just register blocks < 3.
It seems that these plugins actually don't support the iframe. Please see the video at the end of this comment.
There was a problem hiding this comment.
Is this about the "link" format?
There was a problem hiding this comment.
Ok, so it's not really about blocksV3, it seems like a separate discussion and these plugins seems like they're already broken for most new WP installs / site editor...
There was a problem hiding this comment.
That's right. From what I can see, these plugins don't enable their own link formatting outside the post editor.
So the place these plugins don't actually work is in the mobile/tablet views which always run as an iframe editor. With this PR the desktop view will also run as an iframe editor, making the issue more apparent.
There was a problem hiding this comment.
I would like to approve this PR.
Having the editor always work as an iframe is our final goal, and I think the impact on plugin developers is inevitable anyway.
It might be a good idea to ship this PR to WP6.7 and get feedback in the beta phase.
By the way, I saw that @stokesman submitted an issue in the Yoast SEO plugin repository. That issue details one example of how this PR affects plugin developers and what action they need to take.
talldan
added
the
Needs Dev Note
I've added the 'Needs Dev Note' label as dev notes are a good way to communicate this kind of thing to plugin devs. |
👍
And it already has a linked PR 🎉. I’ve been meaning to open an issue for AIOSEO as it’s the other known plugin that would have its existing problem made critically exposed by these changes. Other than that, it seems the subset of plugins with the same problem is very small. They’d be ones that add a format with a text-anchored popover and also add a meta box. Due to meta boxes (currently) disabling the iframe, they may have overlooked compatibility with it. For a plugin that adds a format (with text-anchored popover) and no meta box, surely they’d have tackled it by now. |
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 |
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