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

Show file tree
Hide file tree
Changes from 1 commit
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
15 changes: 15 additions & 0 deletions lib/compat/wordpress-6.5/script-loader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php
/**
* Updates the script-loader.php file
*
* @package gutenberg
*/

add_filter(
'block_editor_settings_all',
static function ( $settings ) {
// We must override what core is passing now.
$settings['themeSupportsAppearanceTools'] = current_theme_supports( 'appearance-tools' );
return $settings;
}
);
1 change: 1 addition & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ function gutenberg_is_experiment_enabled( $name ) {
// WordPress 6.5 compat.
require __DIR__ . '/compat/wordpress-6.5/block-patterns.php';
require __DIR__ . '/compat/wordpress-6.5/class-wp-navigation-block-renderer.php';
require __DIR__ . '/compat/wordpress-6.5/script-loader.php';

// Experimental features.
require __DIR__ . '/experimental/block-editor-settings-mobile.php';
Expand Down
15 changes: 13 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 @@ -92,6 +92,7 @@ function BlockListBlock( {
toggleSelection,
} ) {
const {
themeSupportsAppearanceTools,
themeSupportsLayout,
isTemporarilyEditingAsBlocks,
blockEditingMode,
Expand All @@ -103,6 +104,8 @@ function BlockListBlock( {
getBlockEditingMode,
} = select( blockEditorStore );
return {
themeSupportsAppearanceTools:
getSettings().themeSupportsAppearanceTools,
themeSupportsLayout: getSettings().supportsLayout,
isTemporarilyEditingAsBlocks:
__unstableGetTemporarilyEditingAsBlocks() === clientId,
Expand Down Expand Up @@ -161,6 +164,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 +179,11 @@ function BlockListBlock( {
if ( isAligned ) {
blockEdit = (
<div
className="wp-block"
className={ `wp-block${
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: rather than concatenating manually, would it be worth calling classnames( 'wp-block', className ) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah well spotted! Yes it would 😄

themeSupportsAppearanceTools && isSticky
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the themeSupportsAppearanceTools check? The is-position-sticky classname will only be output if the theme supports sticky position, so I think the position support effectively already does this check in:

gutenberg/packages/block-editor/src/hooks/position.js

Lines 194 to 197 in 4e26178

const [ allowFixed, allowSticky ] = useSettings(
'position.fixed',
'position.sticky'
);

Unless I'm missing something of course!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooooh right that simplifies things a bit!

? ' ' + className
: ''
}` }
data-align={ wrapperProps[ 'data-align' ] }
>
{ blockEdit }
Expand Down Expand Up @@ -221,7 +232,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
1 change: 1 addition & 0 deletions packages/editor/src/components/provider/use-block-editor-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ const BLOCK_EDITOR_SETTINGS = [
'readOnly',
'styles',
'titlePlaceholder',
'themeSupportsAppearanceTools',
'supportsLayout',
'widgetTypesToHideFromLegacyWidgetBlock',
'__unstableHasCustomAppender',
Expand Down
Loading