Skip to content
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 view with meta boxes even with legacy canvas #66706

Merged
merged 5 commits into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions packages/block-editor/src/components/block-canvas/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import clsx from 'clsx';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -33,6 +38,7 @@ export function ExperimentalBlockCanvas( {
styles,
contentRef: contentRefProp,
iframeProps,
enableScroll,
} ) {
useBlockCommands();
const isTabletViewport = useViewportMatch( 'medium', '<' );
Expand All @@ -51,12 +57,17 @@ export function ExperimentalBlockCanvas( {
frameSize: '40px',
}
: {};
const className = clsx(
'block-editor-block-canvas',
enableScroll && 'is-scrollable'
);

if ( ! shouldIframe ) {
return (
<BlockTools
__unstableContentRef={ localRef }
style={ { height, display: 'flex' } }
className={ className }
style={ { height } }
>
<EditorStyles
styles={ styles }
Expand All @@ -67,10 +78,6 @@ export function ExperimentalBlockCanvas( {
ref={ contentRef }
className="editor-styles-wrapper"
tabIndex={ -1 }
style={ {
height: '100%',
width: '100%',
} }
>
{ children }
</WritingFlow>
Expand All @@ -81,6 +88,7 @@ export function ExperimentalBlockCanvas( {
return (
<BlockTools
__unstableContentRef={ localRef }
className={ className }
style={ { height, display: 'flex' } }
>
<Iframe
Expand Down
22 changes: 22 additions & 0 deletions packages/block-editor/src/components/block-canvas/style.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,25 @@
.block-editor-block-canvas.is-scrollable {
overflow: auto;

// Applicable only when legacy (non-iframed).
> .block-editor-writing-flow {
display: flow-root;
min-height: 100%;
box-sizing: border-box;
}

// Applicable only when iframed.
> .block-editor-iframe__container {
display: flex;
flex-direction: column;

> .block-editor-iframe__scale-container {
flex: 1 0 fit-content;
display: flow-root;
}
}
}

iframe[name="editor-canvas"] {
box-sizing: border-box;
width: 100%;
Expand Down
23 changes: 5 additions & 18 deletions packages/edit-post/src/components/layout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,7 @@ function useEditorStyles( ...additionalStyles ) {
] );
}

/**
* @param {Object} props
* @param {boolean} props.isLegacy True when the editor canvas is not in an iframe.
*/
function MetaBoxesMain( { isLegacy } ) {
function MetaBoxesMain() {
const [ isOpen, openHeight, hasAnyVisible ] = useSelect( ( select ) => {
const { get } = select( preferencesStore );
const { isMetaBoxLocationVisible } = select( editPostStore );
Expand Down Expand Up @@ -232,22 +228,15 @@ function MetaBoxesMain( { isLegacy } ) {

const contents = (
<div
className={ clsx(
// The class name 'edit-post-layout__metaboxes' is retained because some plugins use it.
'edit-post-layout__metaboxes',
! isLegacy && 'edit-post-meta-boxes-main__liner'
) }
hidden={ ! isLegacy && isShort && ! isOpen }
// The class name 'edit-post-layout__metaboxes' is retained because some plugins use it.
className="edit-post-layout__metaboxes edit-post-meta-boxes-main__liner"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we combine two CSS classes into one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think since we have to keep edit-post-layout__metaboxes for the sake of plugins we could utilize only that one. Is that what you mean? I suppose that’d be fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. I thought it would be nice to remove the .edit-post-meta-boxes-main__liner selector and put these styles into the .edit-post-layout__metaboxes selector:

.edit-post-meta-boxes-main__liner {
overflow: auto;
// Keep the contents behind the resize handle or details summary.
isolation: isolate;
}
// In case the canvas is not iframe’d.
.edit-post-layout__metaboxes {
clear: both;
}

hidden={ isShort && ! isOpen }
>
<MetaBoxes location="normal" />
<MetaBoxes location="advanced" />
</div>
);

if ( isLegacy ) {
return contents;
}

const isAutoHeight = openHeight === undefined;
let usedMax = '50%'; // Approximation before max has a value.
if ( max !== undefined ) {
Expand Down Expand Up @@ -580,9 +569,7 @@ function Layout( {
}
extraContent={
! isDistractionFree &&
showMetaBoxes && (
<MetaBoxesMain isLegacy={ ! shouldIframe } />
)
showMetaBoxes && <MetaBoxesMain />
}
>
<PostLockedModal />
Expand Down
8 changes: 3 additions & 5 deletions packages/edit-post/src/components/layout/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,9 @@
}

.has-metaboxes .editor-visual-editor {
flex: 1;

&.is-iframed {
isolation: isolate;
}
// Contains z-indexes of children so that the block toolbar will appear behind
// the drop shadow of the meta box pane.
isolation: isolate;
}

// Adjust the position of the notices
Expand Down
3 changes: 2 additions & 1 deletion packages/editor/src/components/editor-interface/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@
}

.editor-visual-editor {
flex: 1 0 auto;
// Fits the height to the parent — flex-shrink ensures it doesn’t create overflow.
flex: 1 1 0%;
}
3 changes: 3 additions & 0 deletions packages/editor/src/components/visual-editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,9 @@ function VisualEditor( {
...deviceStyles,
},
} }
// When there’s no iframe the canvas is the scrolling context and with the
// iframe, device previews may overflow vertically.
enableScroll={ disableIframe || deviceType !== 'Desktop' }
Copy link
Contributor

@t-hamano t-hamano Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there's a way to solve this at a higher level without adding a new prop to the BlockCanvas component. Here's a rough diff, but what do you think?

Details
diff --git a/packages/block-editor/src/components/block-canvas/index.js b/packages/block-editor/src/components/block-canvas/index.js
index ef70c4a26d..dd24a91970 100644
--- a/packages/block-editor/src/components/block-canvas/index.js
+++ b/packages/block-editor/src/components/block-canvas/index.js
@@ -1,8 +1,3 @@
-/**
- * External dependencies
- */
-import clsx from 'clsx';
-
 /**
  * WordPress dependencies
  */
@@ -38,7 +33,6 @@ export function ExperimentalBlockCanvas( {
        styles,
        contentRef: contentRefProp,
        iframeProps,
-       enableScroll,
 } ) {
        useBlockCommands();
        const isTabletViewport = useViewportMatch( 'medium', '<' );
@@ -57,18 +51,10 @@ export function ExperimentalBlockCanvas( {
                                        frameSize: '40px',
                          }
                        : {};
-       const className = clsx(
-               'block-editor-block-canvas',
-               enableScroll && 'is-scrollable'
-       );
 
        if ( ! shouldIframe ) {
                return (
-                       <BlockTools
-                               __unstableContentRef={ localRef }
-                               className={ className }
-                               style={ { height } }
-                       >
+                       <BlockTools __unstableContentRef={ localRef } style={ { height } }>
                                <EditorStyles
                                        styles={ styles }
                                        scope=":where(.editor-styles-wrapper)"
@@ -88,7 +74,6 @@ export function ExperimentalBlockCanvas( {
        return (
                <BlockTools
                        __unstableContentRef={ localRef }
-                       className={ className }
                        style={ { height, display: 'flex' } }
                >
                        <Iframe
diff --git a/packages/block-editor/src/components/block-canvas/style.scss b/packages/block-editor/src/components/block-canva
s/style.scss
index 52c204407e..840ef3644c 100644
--- a/packages/block-editor/src/components/block-canvas/style.scss
+++ b/packages/block-editor/src/components/block-canvas/style.scss
@@ -1,4 +1,4 @@
-.block-editor-block-canvas.is-scrollable {
+.edit-post-visual-editor.is-scrollable {
        overflow: auto;
 
        // Applicable only when legacy (non-iframed).
diff --git a/packages/editor/src/components/visual-editor/index.js b/packages/editor/src/components/visual-editor/index.js
index 5a75355e08..122252ea4a 100644
--- a/packages/editor/src/components/visual-editor/index.js
+++ b/packages/editor/src/components/visual-editor/index.js
@@ -385,6 +385,7 @@ function VisualEditor( {
                                        'has-padding': isFocusedEntity || enableResizing,
                                        'is-resizable': enableResizing,
                                        'is-iframed': ! disableIframe,
+                                       'is-scrollable': disableIframe || deviceType !== 'Desktop',
                                }
                        ) }
                >
@@ -406,9 +407,6 @@ function VisualEditor( {
                                                        ...deviceStyles,
                                                },
                                        } }
-                                       // When there’s no iframe the canvas is the scrolling context and with the
-                                       // iframe, device previews may overflow vertically.
-                                       enableScroll={ disableIframe || deviceType !== 'Desktop' }
                                >
                                        { themeSupportsLayout &&
                                                ! themeHasDisabledLayoutStyles &&

Copy link
Contributor Author

@stokesman stokesman Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for suggesting this. I’ve tried it. The only thing is actually having the visual editor’s root element as scrollable is trickier as it won’t contain the margins of the device previews. That’s because the block canvas’ root element has an inline-styled height of 100% (as specified by VisualEditor) and that’s important for fluid transitions to/from device previews. So to get the margins to be included in the overflow the height has to be auto while scrollable but then it breaks the transitions to a device preview.

Screen recording to demonstrate issues with VisualEditor as the scrolling context
visual-editor-scroll-challenges.mp4

Still, it looks like we can avoid adding a prop to ExperimentalBlockCanvas d008922. The component still needs a class added to target through CSS. It makes for what may be considered slightly worse style encapsulation because now the editor package is relying on a class applied in the block-editor package. For that reason, I think adding the prop may be slightly "cleaner" 🤷.

>
{ themeSupportsLayout &&
! themeHasDisabledLayoutStyles &&
Expand Down
9 changes: 3 additions & 6 deletions packages/editor/src/components/visual-editor/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
position: relative;
display: flex;
background-color: $gray-300;
// Allows the height to fit the parent container and avoids parent scrolling contexts from
// having overflow due to popovers of block tools.
overflow: hidden;

// Centralize the editor horizontally (flex-direction is column).
align-items: center;
Expand All @@ -14,12 +17,6 @@
padding: $grid-unit-30 $grid-unit-30 0;
}

// In the iframed canvas this keeps extra scrollbars from appearing (when block toolbars overflow). In the
// legacy (non-iframed) canvas, overflow must not be hidden in order to maintain support for sticky positioning.
&.is-iframed {
overflow: hidden;
}

// The button element easily inherits styles that are meant for the editor style.
// These rules enhance the specificity to reduce that inheritance.
// This is duplicated in edit-site.
Expand Down
Loading