-
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
Split content view with open metaboxes #64351
Changes from 16 commits
7f30335
0ce4b1a
1b62663
4952671
cf8294e
c9145ba
6f62680
ed739ad
9ed8afa
c1d8d0f
c672254
e845117
8390679
80a90e3
3da7979
c31b7cf
64590b2
8df9313
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,70 @@ | ||
.edit-post-layout__metaboxes { | ||
flex-shrink: 0; | ||
clear: both; | ||
.edit-post-meta-boxes-main { | ||
filter: drop-shadow(0 -1px rgba($color: #000, $alpha: 0.133)); // 0.133 = $gray-200 but with alpha. | ||
background-color: $white; | ||
clear: both; // This is seemingly only needed in case the canvas is not iframe’d. | ||
|
||
&:not(details) { | ||
padding-top: 23px; | ||
max-height: 100%; | ||
|
||
&:not(.has-user-size) { | ||
max-height: 50% !important; | ||
} | ||
} | ||
|
||
// The component renders as a details element in short viewports. | ||
&:is(details) { | ||
& > summary { | ||
cursor: pointer; | ||
color: $gray-900; | ||
background-color: $white; | ||
height: $button-size-compact; | ||
line-height: $button-size-compact; | ||
font-size: 13px; | ||
padding-left: $grid-unit-30; | ||
box-shadow: 0 $border-width $gray-300; | ||
} | ||
|
||
&[open] > summary { | ||
position: sticky; | ||
top: 0; | ||
z-index: 1; | ||
} | ||
} | ||
|
||
& .components-resizable-box__handle-top { | ||
top: 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’ve got a branch that does it. It’s looking like about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good — feel free to ping on that separate PR |
||
box-shadow: 0 $border-width $gray-300; | ||
} | ||
& .components-resizable-box__side-handle::before { | ||
border-radius: 0; | ||
top: 0; | ||
height: $border-width; | ||
} | ||
& .components-resizable-box__handle::after { | ||
background-color: $gray-300; | ||
box-shadow: none; | ||
border-radius: 4px; | ||
height: $grid-unit-05; | ||
top: calc(50% - #{$grid-unit-05} / 2); | ||
width: 100px; | ||
right: calc(50% - 50px); | ||
} | ||
} | ||
|
||
.edit-post-meta-boxes-main__liner { | ||
overflow: auto; | ||
max-height: 100%; | ||
// Keep the contents behind the resize handle or details summary. | ||
isolation: isolate; | ||
} | ||
|
||
.has-metaboxes .editor-visual-editor { | ||
flex: 1; | ||
|
||
&.is-iframed { | ||
isolation: isolate; | ||
} | ||
} | ||
|
||
// Adjust the position of the notices | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,19 +6,13 @@ import { useSelect } from '@wordpress/data'; | |
import { store as blocksStore } from '@wordpress/blocks'; | ||
import { store as blockEditorStore } from '@wordpress/block-editor'; | ||
|
||
/** | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In which case the iframe is not used now? Can't we just remove this hook entirely There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this about the "link" format? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
const { | ||
isBlockBasedTheme, | ||
hasV3BlocksOnly, | ||
isEditingTemplate, | ||
hasMetaBoxes, | ||
isZoomOutMode, | ||
} = useSelect( ( select ) => { | ||
const { getEditorSettings, getCurrentPostType } = select( editorStore ); | ||
|
@@ -31,14 +25,13 @@ export function useShouldIframe() { | |
return type.apiVersion >= 3; | ||
} ), | ||
isEditingTemplate: getCurrentPostType() === 'wp_template', | ||
hasMetaBoxes: select( editPostStore ).hasMetaBoxes(), | ||
isZoomOutMode: __unstableGetEditorMode() === 'zoom-out', | ||
}; | ||
}, [] ); | ||
|
||
return ( | ||
( ( hasV3BlocksOnly || ( isGutenbergPlugin && isBlockBasedTheme ) ) && | ||
! hasMetaBoxes ) || | ||
hasV3BlocksOnly || | ||
( isGutenbergPlugin && isBlockBasedTheme ) || | ||
isEditingTemplate || | ||
isZoomOutMode | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -379,7 +379,6 @@ export const isMetaBoxLocationVisible = createRegistrySelector( | |
isMetaBoxLocationActive( state, location ) && | ||
getMetaBoxesPerLocation( state, location )?.some( ( { id } ) => { | ||
return select( editorStore ).isEditorPanelEnabled( | ||
state, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was making |
||
`meta-box-${ id }` | ||
); | ||
} ) | ||
|
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.
Nit: The same style is defined here, so is this prop unnecessary?
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.
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.