-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Layout: Add a disable-layout-styles theme supports flag to opt out of all layout styles #42544
Changes from 3 commits
a6048ae
e825920
0bef544
d77120e
6b6dadb
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 |
---|---|---|
|
@@ -118,13 +118,15 @@ export default function VisualEditor( { styles } ) { | |
( select ) => select( editPostStore ).hasMetaBoxes(), | ||
[] | ||
); | ||
const { themeSupportsLayout, assets } = useSelect( ( select ) => { | ||
const _settings = select( blockEditorStore ).getSettings(); | ||
return { | ||
themeSupportsLayout: _settings.supportsLayout, | ||
assets: _settings.__unstableResolvedAssets, | ||
}; | ||
}, [] ); | ||
const { themeHasDisabledLayoutStyles, themeSupportsLayout, assets } = | ||
useSelect( ( select ) => { | ||
const _settings = select( blockEditorStore ).getSettings(); | ||
return { | ||
themeHasDisabledLayoutStyles: _settings.disableLayoutStyles, | ||
themeSupportsLayout: _settings.supportsLayout, | ||
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. Why a new key config when we already have 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. The names are unfortunately similar, but |
||
assets: _settings.__unstableResolvedAssets, | ||
}; | ||
}, [] ); | ||
const { clearSelectedBlock } = useDispatch( blockEditorStore ); | ||
const { setIsEditingTemplate } = useDispatch( editPostStore ); | ||
const desktopCanvasStyles = { | ||
|
@@ -241,13 +243,17 @@ export default function VisualEditor( { styles } ) { | |
assets={ assets } | ||
style={ { paddingBottom } } | ||
> | ||
{ themeSupportsLayout && ! isTemplateMode && ( | ||
<LayoutStyle | ||
selector=".edit-post-visual-editor__post-title-wrapper, .block-editor-block-list__layout.is-root-container" | ||
layout={ defaultLayout } | ||
layoutDefinitions={ defaultLayout?.definitions } | ||
/> | ||
) } | ||
{ themeSupportsLayout && | ||
! themeHasDisabledLayoutStyles && | ||
! isTemplateMode && ( | ||
<LayoutStyle | ||
selector=".edit-post-visual-editor__post-title-wrapper, .block-editor-block-list__layout.is-root-container" | ||
layout={ defaultLayout } | ||
layoutDefinitions={ | ||
defaultLayout?.definitions | ||
} | ||
/> | ||
) } | ||
{ ! isTemplateMode && ( | ||
<div | ||
className="edit-post-visual-editor__post-title-wrapper" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,11 +21,13 @@ import { | |
__EXPERIMENTAL_ELEMENTS as ELEMENTS, | ||
getBlockTypes, | ||
} from '@wordpress/blocks'; | ||
import { useSelect } from '@wordpress/data'; | ||
import { useEffect, useState, useContext } from '@wordpress/element'; | ||
import { getCSSRules } from '@wordpress/style-engine'; | ||
import { | ||
__unstablePresetDuotoneFilter as PresetDuotoneFilter, | ||
__experimentalGetGapCSSValue as getGapCSSValue, | ||
store as blockEditorStore, | ||
} from '@wordpress/block-editor'; | ||
|
||
/** | ||
|
@@ -536,7 +538,8 @@ export const toStyles = ( | |
tree, | ||
blockSelectors, | ||
hasBlockGapSupport, | ||
hasFallbackGapSupport | ||
hasFallbackGapSupport, | ||
skipLayoutStyles = false | ||
) => { | ||
const nodesWithStyles = getNodesWithStyles( tree, blockSelectors ); | ||
const nodesWithSettings = getNodesWithSettings( tree, blockSelectors ); | ||
|
@@ -612,7 +615,10 @@ export const toStyles = ( | |
} | ||
|
||
// Process blockGap and layout styles. | ||
if ( ROOT_BLOCK_SELECTOR === selector || hasLayoutSupport ) { | ||
if ( | ||
! skipLayoutStyles && | ||
( ROOT_BLOCK_SELECTOR === selector || hasLayoutSupport ) | ||
) { | ||
ruleset += getLayoutStyles( { | ||
tree, | ||
style: styles, | ||
|
@@ -761,6 +767,22 @@ export const getBlockSelectors = ( blockTypes ) => { | |
return result; | ||
}; | ||
|
||
/** | ||
* Determines whether or not the theme has disabled all layout styles output. | ||
* | ||
* This feature only disables the output of layout styles, | ||
* the controls for adjusting layout will still be available in the editor. | ||
* Themes that use this feature commit to providing their own styling for layout features. | ||
* | ||
* @return {boolean} Whether or not the theme opts-in to disable all layout styles. | ||
*/ | ||
function useThemeHasDisabledLayoutStyles() { | ||
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. Could the site editor package access the hook from the block editor package instead of duplicating it? 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 a good question — I wasn't sure if we wanted to expose it for use anywhere else so left it as a duplication for now. But if it's an official theme support, then yes, it probably makes better sense to have it defined once in the block editor package, and then export it 👍 I'll have a play. 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. After taking another look, I think it makes the code a little clearer if we don't actually use the hook abstraction. It didn't add much, and meant that if you wanted to see what it was actually checking you'd have to look at the implementation of the hook. In 6b6dadb I've switched to inlining the settings checks, so that it's closer to the feature name, which I think on balance, is a little easier to read. Happy to change it, though, if you think there's a better way to handle it! |
||
return useSelect( ( select ) => { | ||
const { getSettings } = select( blockEditorStore ); | ||
return !! getSettings().disableLayoutStyles; | ||
} ); | ||
} | ||
|
||
export function useGlobalStylesOutput() { | ||
const [ stylesheets, setStylesheets ] = useState( [] ); | ||
const [ settings, setSettings ] = useState( {} ); | ||
|
@@ -769,6 +791,7 @@ export function useGlobalStylesOutput() { | |
const [ blockGap ] = useSetting( 'spacing.blockGap' ); | ||
const hasBlockGapSupport = blockGap !== null; | ||
const hasFallbackGapSupport = ! hasBlockGapSupport; // This setting isn't useful yet: it exists as a placeholder for a future explicit fallback styles support. | ||
const skipLayoutStyles = useThemeHasDisabledLayoutStyles(); | ||
|
||
useEffect( () => { | ||
if ( ! mergedConfig?.styles || ! mergedConfig?.settings ) { | ||
|
@@ -784,7 +807,8 @@ export function useGlobalStylesOutput() { | |
mergedConfig, | ||
blockSelectors, | ||
hasBlockGapSupport, | ||
hasFallbackGapSupport | ||
hasFallbackGapSupport, | ||
skipLayoutStyles | ||
); | ||
const filters = toSvgFilters( mergedConfig, blockSelectors ); | ||
setStylesheets( [ | ||
|
@@ -799,7 +823,12 @@ export function useGlobalStylesOutput() { | |
] ); | ||
setSettings( mergedConfig.settings ); | ||
setSvgFilters( filters ); | ||
}, [ hasBlockGapSupport, hasFallbackGapSupport, mergedConfig ] ); | ||
}, [ | ||
hasBlockGapSupport, | ||
hasFallbackGapSupport, | ||
mergedConfig, | ||
skipLayoutStyles, | ||
] ); | ||
|
||
return [ stylesheets, settings, svgFilters, hasBlockGapSupport ]; | ||
} |
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.
There's also a bunch of layout-related logic towards the top of this function. Would it make sense to return early instead of adding this condition here?
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.
Thanks for testing! In this case, I don't think we can return early, because we still want the Layout support's semantic classnames to be added to the output which happens at line 260. Part of the feedback that inspired this PR was that there are Classic themes that would like the semantic classnames like layout type and content justification to be available, but not the generated Layout styles themselves.
It does make it a bit of an awkward place for the check to live, though! I suppose we could move it up to line 228 so that this
if
block includes the$gap_value
declaration and everything below that?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.
Oooh, of course, that makes sense 🤦 Yeah, I guess we could move it up a little bit, so there are no unnecessary calculations performed.
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.
I've moved it up a little in d77120e, which feels a bit more logical to me now, thanks for the suggestion! Also added a comment about the classnames so it's clearer why we're not returning early.
Separately to this PR, now that we've got a fair few classes being generated it might be worth us re-exploring splitting out the classnames logic to a separate function to make it a bit easier to read. We had it in a separate function for a little while but reverted in #41885 so that we could give it some more thought before committing to a particular approach that we then need to continue to support. But 🤞 we can settle on an approach before 6.1.