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

Fix sticky position in classic themes with appearance tools support #56743

Merged
merged 3 commits into from
Dec 5, 2023
Merged

Fix sticky position in classic themes with appearance tools support #56743

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c0d810
Fix sticky position in classic themes with appearance tools support
tellthemachines Dec 4, 2023
1989f60
Remove editor setting to detect appearance tools support
tellthemachines Dec 5, 2023
3981184
Delete unused file
tellthemachines Dec 5, 2023
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
8 changes: 6 additions & 2 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ function BlockListBlock( {
!! wrapperProps[ 'data-align' ] &&
! themeSupportsLayout;

// Support for sticky position in classic themes with alignment wrappers.

const isSticky = className?.includes( 'is-position-sticky' );
andrewserong marked this conversation as resolved.
Show resolved Hide resolved

// For aligned blocks, provide a wrapper element so the block can be
// positioned relative to the block column.
// This is only kept for classic themes that don't support layout
Expand All @@ -172,7 +176,7 @@ function BlockListBlock( {
if ( isAligned ) {
blockEdit = (
<div
className="wp-block"
className={ classnames( 'wp-block', isSticky && className ) }
data-align={ wrapperProps[ 'data-align' ] }
>
{ blockEdit }
Expand Down Expand Up @@ -221,7 +225,7 @@ function BlockListBlock( {
isTemporarilyEditingAsBlocks,
},
dataAlign && themeSupportsLayout && `align${ dataAlign }`,
className
! ( dataAlign && isSticky ) && className
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not an issue, but if there are other block supports that generate a className (like element colors for link colors) then these classnames will also be moved up to the align wrapper if the block has been set to sticky? I think this is most likely fine, though, as we're already moving those classnames that need to act on the inner wrapper down to the inner wrapper to handle block gap, so it's all working nicely for me locally.

image

I.e. I can both set to sticky and set a custom block gap 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I tried a few supports in testing and the others didn't seem to be included in the className variable. I'll check again. Also now I'm wondering if we can leverage classname swapping to fix the issue with background color overlapping background image too:

Screenshot 2023-12-05 at 3 01 49 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the only other classname being transferred to the alignment wrapper is wp-elements-x, which isn't problematic because its styles don't target direct descendants. If this did become a problem later on we could perhaps filter out the position classnames, but best avoid adding extra logic if it isn't needed.

What I did notice is that in the editor, when testing with a single group block on the page. position and layout classnames were identical wp-container-0. In practice that can't cause problems because applying sticky position to the inner group container won't do anything, and neither will the block spacing rules override margins set on the block. To be on the safe side though, we might want to consider changing the layout class in the editor to match the front end wp-container-core-group-layout-x (and possibly a similar specificity could be applied to the position class at some point). I'd do that as a separate PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: the background styling in the screenshot above is specific to TT1, so I guess it's fine to leave as is. Imo it's not worth the overhead of trying to attach the background image to the inner container, because it's pretty easy to fix this in the UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. The less we need to mutate for the classic themes case the better, if possible. We can always continue exploring in follow-ups if need be, too!

),
wrapperProps: restWrapperProps,
isAligned,
Expand Down
Loading