Repository navigation
Enable text alignment to be set by viewport state #80037
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
Changes from all commits
3359738
fcec83c
289bebd
d9afd72
88fa962
442456e
76e39d6
3b3d9ce
2f47f61
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 |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| https://github.com/WordPress/wordpress-develop/pull/12489 | ||
|
|
||
| * https://github.com/WordPress/gutenberg/pull/80037 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -301,29 +301,34 @@ function LayoutPanelPure( { | |
| const settings = useBlockSettings( blockName ); | ||
| // Block settings come from theme.json under settings.[blockName]. | ||
| const { layout: layoutSettings } = settings; | ||
| const { themeSupportsLayout, activeBlockVariation, selectedState } = | ||
| useSelect( | ||
| ( select ) => { | ||
| const blockEditorSelect = select( blockEditorStore ); | ||
| const { getBlockAttributes, getSettings } = blockEditorSelect; | ||
| const { getSelectedBlockStyleState } = | ||
| unlock( blockEditorSelect ); | ||
| return { | ||
| activeBlockVariation: select( | ||
| blocksStore | ||
| ).getActiveBlockVariation( | ||
| blockName, | ||
| getBlockAttributes( clientId ) || {}, | ||
| 'block' | ||
| ), | ||
| themeSupportsLayout: getSettings().supportsLayout, | ||
| selectedState: | ||
| getSelectedBlockStyleState?.( clientId ) ?? | ||
| DEFAULT_BLOCK_STYLE_STATE, | ||
| }; | ||
| }, | ||
| [ blockName, clientId ] | ||
| ); | ||
| const { | ||
| themeSupportsLayout, | ||
| activeBlockVariation, | ||
| selectedState, | ||
| isResponsiveEditing, | ||
| } = useSelect( | ||
| ( select ) => { | ||
| const blockEditorSelect = select( blockEditorStore ); | ||
| const { getBlockAttributes, getSettings } = blockEditorSelect; | ||
| const { | ||
| getSelectedBlockStyleState, | ||
| isResponsiveEditing: getIsResponsiveEditing, | ||
| } = unlock( blockEditorSelect ); | ||
| return { | ||
| activeBlockVariation: select( | ||
| blocksStore | ||
| ).getActiveBlockVariation( | ||
| blockName, | ||
| getBlockAttributes( clientId ) || {}, | ||
| 'block' | ||
| ), | ||
| themeSupportsLayout: getSettings().supportsLayout, | ||
| selectedState: getSelectedBlockStyleState( clientId ), | ||
| isResponsiveEditing: getIsResponsiveEditing(), | ||
| }; | ||
| }, | ||
| [ blockName, clientId ] | ||
| ); | ||
|
|
||
| const blockEditingMode = useBlockEditingMode(); | ||
| const isViewportLayoutState = | ||
|
|
@@ -573,6 +578,13 @@ function LayoutPanelPure( { | |
| layout={ usedLayout } | ||
| onChange={ onChangeLayout } | ||
| layoutBlockSupport={ layoutBlockSupport } | ||
| controlsGroup={ | ||
| isResponsiveEditing && | ||
| hasViewportBlockStyleState( selectedState ) && | ||
| ! hasPseudoBlockStyleState( selectedState ) | ||
|
Contributor
Author
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. Layout doesn't support pseudo states (it hasn't had to so far because no block with pseudo states has layout support; this may change in the future of course)
Contributor
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. I think we should be a little bit careful about these clauses, as when other style states are introduced (e.g. custom/current), the dev possibly has to remember to add Possibly we should have a selector Not urgent though, I think we might want to tidy some of these things up post 7.1 to avoid refactors / merge conflicts right now.
Contributor
Author
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. While for pseudo states (and for "current" I guess) we can be fairly sure we don't need layout controls, if other custom states are introduced some of them might require layout support. Probably better to reexamine this later 😄
Contributor
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. Good point, some declarative way of opting in per support probably makes more sense then. |
||
| ? 'style-state' | ||
| : 'block' | ||
| } | ||
| name={ blockName } | ||
| clientId={ clientId } | ||
| /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,11 @@ import { | |
| __EXPERIMENTAL_ELEMENTS as ELEMENTS, | ||
| } from '@wordpress/blocks'; | ||
| import { useInstanceId } from '@wordpress/compose'; | ||
| import { getCSSRules, compileCSS } from '@wordpress/style-engine'; | ||
| import { | ||
| getCSSRules, | ||
| compileCSS, | ||
| getCSSValueFromRawStyle, | ||
| } from '@wordpress/style-engine'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
|
|
@@ -201,6 +205,32 @@ function getStateFallbackDimensionStyles( stateStyles ) { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Returns CSS for text alignment in a block instance state style object. | ||
| * | ||
| * Text alignment is not generated by the style engine because base block | ||
| * alignment is serialized as a class name. State styles need a declaration | ||
| * instead so viewport and pseudo-state values can override the default class. | ||
| * | ||
| * @param {Object} stateStyles State style object. | ||
| * @param {string} selector CSS selector for the generated style. | ||
| * @return {string|undefined} CSS string with the text alignment declaration. | ||
| */ | ||
| function getStateTextAlignCSS( stateStyles, selector ) { | ||
| const textAlign = stateStyles?.typography?.textAlign; | ||
|
|
||
| if ( typeof textAlign !== 'string' || ! textAlign ) { | ||
| return undefined; | ||
| } | ||
|
Comment on lines
+222
to
+224
Contributor
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. This is such a tiny nit, and likely doesn't even matter because I don't think (If you're not making any other changes to the PR, don't worry about it)
Contributor
Author
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. it might be that we can remove the trim from the PHP instead 😄
Contributor
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. Definitely not a blocker!! |
||
|
|
||
| const declaration = `text-align: ${ getCSSValueFromRawStyle( | ||
| textAlign | ||
| ) } !important`; | ||
| return selector | ||
| ? `${ selector } { ${ declaration }; }` | ||
| : `${ declaration };`; | ||
| } | ||
|
|
||
| /** | ||
| * Generates CSS for a block instance state style object. | ||
| * | ||
|
|
@@ -224,12 +254,13 @@ export function getStateStylesCSS( stateStyles, selector ) { | |
| const fallbackCSS = fallbackBorderStyles | ||
| ? compileCSS( fallbackBorderStyles, { selector } ) | ||
| : undefined; | ||
| const textAlignCSS = getStateTextAlignCSS( stateStyles, selector ); | ||
| const backgroundResetCSS = getStateBackgroundResetCSS( | ||
| stateStyles, | ||
| selector | ||
| ); | ||
|
|
||
| return [ importantCSS, fallbackCSS, backgroundResetCSS ] | ||
| return [ importantCSS, textAlignCSS, fallbackCSS, backgroundResetCSS ] | ||
| .filter( Boolean ) | ||
| .join( '\n' ); | ||
| } | ||
|
|
||
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.
Sorry, this caught my eye, and I couldn't help myself, so here's a minor follow-up #80176.