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.

Sign up for GitHub

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

Jump to bottom

[Block Editor]: Try marking BlockList as non-Root component #35932

Closed
wants to merge 1 commit into from
Closed

[Block Editor]: Try marking BlockList as non-Root component #35932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 7 additions & 2 deletions packages/block-editor/src/components/block-list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ const elementContext = createContext();

export const IntersectionObserver = createContext();

function Root( { className, ...settings } ) {
function Root( {
className,
__experimentalIsRootContainer = true,
...settings
} ) {
const [ element, setElement ] = useState();
const isLargeViewport = useViewportMatch( 'medium' );
const { isOutlineMode, isFocusMode, isNavigationMode } = useSelect(
Expand All @@ -55,7 +59,8 @@ function Root( { className, ...settings } ) {
useInBetweenInserter(),
setElement,
] ),
className: classnames( 'is-root-container', className, {
className: classnames( className, {
'is-root-container': __experimentalIsRootContainer,
'is-outline-mode': isOutlineMode,
'is-focus-mode': isFocusMode && isLargeViewport,
'is-navigate-mode': isNavigationMode,
Expand Down
4 changes: 0 additions & 4 deletions packages/block-editor/src/components/block-pattern-setup/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,5 @@
}
}
}

.block-list-appender {
display: none;
}
}
}
13 changes: 11 additions & 2 deletions packages/block-editor/src/components/block-preview/auto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ import { store } from '../../store';
// This is used to avoid rendering the block list if the sizes change.
let MemoizedBlockList;

function AutoBlockPreview( { viewportWidth, __experimentalPadding } ) {
function AutoBlockPreview( {
viewportWidth,
__experimentalPadding,
__experimentalIsRootContainer,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm hesitant about this prop, it's very weird to be honest. it's a hack because the issue here is not really the real issue here, the issue is the added div with its own layout. So while I'm ok with this PR, I was wondering if we can borrow the useBlockPreview from the other PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me too. I was kind of expecting to close it to be honest 😄

I was wondering if we can borrow the useBlockPreview from the other PR?

I'll try this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if we can borrow the useBlockPreview from the other PR?

Thanks for exploring that! I quite like the useBlockPreview we came up with in that other PR, so it'd be cool if we can include it here 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andrewserong would you be interested in taking over this or maybe better creating a PR with useBlockPreview? I tried this a bit and it seems most of the code needed had been implemented by you in: #35863.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ntsekouras sure, I'm happy either take this over or create another PR. The only thing is I won't be able to start on it until later this week in case there's urgency in getting this in sooner. If not, then I can pick it up 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds great, thanks! 💯 This is a bug, so there is time to fix it and include it in 5.9.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wrapping up for the week, but I've started on a fix and pushed a draft PR in #36431 that appears to work okay so far — I haven't had a chance to test it properly or see if it can be streamlined a bit, so I've left it as a draft PR for the moment, and will continue on with it on Monday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's great Andrew! Thanks! I'll close this one and we can use your new PR.

} ) {
const [
containerResizeListener,
{ width: containerWidth },
Expand Down Expand Up @@ -65,7 +69,12 @@ function AutoBlockPreview( { viewportWidth, __experimentalPadding } ) {
} }
>
{ contentResizeListener }
<MemoizedBlockList renderAppender={ false } />
<MemoizedBlockList
renderAppender={ false }
__experimentalIsRootContainer={
__experimentalIsRootContainer
}
/>
</Iframe>
</Disabled>
</div>
Expand Down
11 changes: 10 additions & 1 deletion packages/block-editor/src/components/block-preview/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export function BlockPreview( {
viewportWidth = 1200,
__experimentalLive = false,
__experimentalOnClick,
__experimentalIsRootContainer = true,
} ) {
const originalSettings = useSelect(
( select ) => select( blockEditorStore ).getSettings(),
Expand All @@ -40,11 +41,19 @@ export function BlockPreview( {
return (
<BlockEditorProvider value={ renderedBlocks } settings={ settings }>
{ __experimentalLive ? (
<LiveBlockPreview onClick={ __experimentalOnClick } />
<LiveBlockPreview
onClick={ __experimentalOnClick }
__experimentalIsRootContainer={
__experimentalIsRootContainer
}
/>
) : (
<AutoHeightBlockPreview
viewportWidth={ viewportWidth }
__experimentalPadding={ __experimentalPadding }
__experimentalIsRootContainer={
__experimentalIsRootContainer
}
/>
) }
</BlockEditorProvider>
Expand Down
12 changes: 10 additions & 2 deletions packages/block-editor/src/components/block-preview/live.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import { Disabled } from '@wordpress/components';
*/
import BlockList from '../block-list';

export default function LiveBlockPreview( { onClick } ) {
export default function LiveBlockPreview( {
onClick,
__experimentalIsRootContainer,
} ) {
return (
<div
tabIndex={ 0 }
Expand All @@ -17,7 +20,12 @@ export default function LiveBlockPreview( { onClick } ) {
onKeyPress={ onClick }
>
<Disabled>
<BlockList />
<BlockList
Copy link
Contributor Author

@ntsekouras ntsekouras Oct 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also, out of scope of this PR, but @ellatrix should we iframe this as well? It feels like it should be a part of #28165.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the late reply. It should be, yes. What's the difference between live and not live?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

live just uses BlockList whereas the auto preview scale the preview with the help of useResizeObserver.

renderAppender={ false }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not related to the actual problem, but noticed it here and is connected with the style removal here.

__experimentalIsRootContainer={
__experimentalIsRootContainer
}
/>
</Disabled>
</div>
);
Expand Down
1 change: 1 addition & 0 deletions packages/block-library/src/post-template/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ export default function PostTemplateEdit( {
__experimentalOnClick={ () =>
setActiveBlockContext( blockContext )
}
__experimentalIsRootContainer={ false }
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the additional "div" added by "BlockPreview" won't this break alignments as well? For the active post Id, there's no addition div if I'm not wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parent div just fills all the available space from its parent container. What we want to avoid is the generated style for root-container:

.editor-styles-wrapper .block-editor-block-list__layout.is-root-container > * {
    max-width: [content width];
    margin-left: auto;
    margin-right: auto;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, but the problem is that if there's a block that is full aligned or wide aligned... inside the previewed blocks, they won't get the layout styles properly, so they won't show as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand why they shouldn't work. If a block has specific layout, it's styles should be rendered properly.

The problem seems to be that they don't work right now as the layout styles are not rendered in PostContent. This PR with the LivePreview PR in readonly state seems to do the trick.

I don't know if I missing something here..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a video with both changes applied(isRootContainer and PostContent LivePreview)

Screen.Recording.2021-10-26.at.11.54.51.AM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

Just compare the DOM output of an active "post id" with an inactive one, you'll see an extra div for the inactive one that disappears as soon as it becomes active.

Right now, in this PR it doesn't break anything because there's an "li" container for both situations that doesn't define any layout but if ever that "li" becomes say a "Post block" where you'd be able to define a "layout", it would break (it's the case in the other PR).

I'm fine if land this PR as is but since we need to solve the container-less preview anyway for the other PR, I was wondering if we could use the same technique here which would make the __experimentalIsRootContainer unnecessary.

Anyway not important for the Current PR as there's no layout in the container of the "preview"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, in this PR it doesn't break anything because there's an "li" container for both situations that doesn't define any layout but if ever that "li" becomes say a "Post block" where you'd be able to define a "layout", it would break

I get what you mean now.

I was wondering if we could use the same technique here which would make the __experimentalIsRootContainer unnecessary.

What technique are you referring to?

Copy link
Contributor

Choose a reason for hiding this comment

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

What technique are you referring to?

I mean if we find a way to use BlockPreview without extra wrapper.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've had a go at conditionally removing the extra wrapper in BlockPreview by adding an __experimentalAsButton prop in #35863 in this commit: 0b77514, it seems to work okay so far, but let me know if you think it'll cause any issues

/>
</li>
) }
Expand Down