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

Allow themes to provide empty values for color.duotone and spacing.units #33280

Merged
merged 6 commits into from
Jul 8, 2021
Merged

Allow themes to provide empty values for color.duotone and spacing.units #33280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
22b3bd3
Add tests for empty incoming presets
oandregal Jul 8, 2021
456a7be
Add tests for null incoming presets
oandregal Jul 8, 2021
f62c32a
Empty arrays are valid input data that need to be merged as well
oandregal Jul 8, 2021
05fc7b2
Layout.units were not being considered for merging
oandregal Jul 8, 2021
fd1fc57
Add tests for layout.units
oandregal Jul 8, 2021
2d19440
Substitute layout.units per spacing.units
oandregal Jul 8, 2021
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
1 change: 0 additions & 1 deletion lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,7 +1073,6 @@ public function merge( $incoming ) {
// For leaf values that are arrays it will use the numeric indexes for replacement.
// In those cases, we want to replace the existing with the incoming value, if it exists.
$to_replace = array();
$to_replace[] = array( 'layout', 'units' );
$to_replace[] = array( 'spacing', 'units' );
$to_replace[] = array( 'color', 'duotone' );
foreach ( self::VALID_ORIGINS as $origin ) {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/letter-spacing-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import useSetting from '../../components/use-setting';
*/
export default function LetterSpacingControl( { value, onChange } ) {
const units = useCustomUnits( {
availableUnits: useSetting( 'layout.units' ) || [ 'px', 'em', 'rem' ],
availableUnits: useSetting( 'spacing.units' ) || [ 'px', 'em', 'rem' ],
defaultValues: { px: '2', em: '.2', rem: '.2' },
} );
return (
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/border-width.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export const BorderWidthEdit = ( props ) => {
};

const units = useCustomUnits( {
availableUnits: useSetting( 'layout.units' ) || [ 'px', 'em', 'rem' ],
availableUnits: useSetting( 'spacing.units' ) || [ 'px', 'em', 'rem' ],
} );

return (
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/layout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ function LayoutPanel( { setAttributes, attributes } ) {
}, [] );

const units = useCustomUnits( {
availableUnits: useSetting( 'layout.units' ) || [
availableUnits: useSetting( 'spacing.units' ) || [
'%',
'px',
'em',
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/column/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ function ColumnEdit( {
} );

const units = useCustomUnits( {
availableUnits: useSetting( 'layout.units' ) || [
availableUnits: useSetting( 'spacing.units' ) || [
'%',
'px',
'em',
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/column/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function ColumnEdit( {
const [ widthUnit, setWidthUnit ] = useState( valueUnit || '%' );

const units = useCustomUnits( {
availableUnits: useSetting( 'layout.units' ) || [
availableUnits: useSetting( 'spacing.units' ) || [
'%',
'px',
'em',
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/columns/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ function ColumnsEditContainer( {
const { width } = sizes || {};

const units = useCustomUnits( {
availableUnits: useSetting( 'layout.units' ) || [
availableUnits: useSetting( 'spacing.units' ) || [
'%',
'px',
'em',
Expand Down
19 changes: 0 additions & 19 deletions phpunit/class-wp-theme-json-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,9 +733,6 @@ public function test_merge_incoming_data_empty_presets() {
),
),
),
'layout' => array(
'units' => array( 'px', 'em' ),
),
'spacing' => array(
'units' => array( 'px', 'em' ),
),
Expand All @@ -761,9 +758,6 @@ public function test_merge_incoming_data_empty_presets() {
'gradients' => array(),
'palette' => array(),
),
'layout' => array(
'units' => array(),
),
'spacing' => array(
'units' => array(),
),
Expand All @@ -788,9 +782,6 @@ public function test_merge_incoming_data_empty_presets() {
'theme' => array(),
),
),
'layout' => array(
'units' => array(),
),
'spacing' => array(
'units' => array(),
),
Expand Down Expand Up @@ -830,9 +821,6 @@ public function test_merge_incoming_data_null_presets() {
),
),
),
'layout' => array(
'units' => array( 'px', 'em' ),
),
'spacing' => array(
'units' => array( 'px', 'em' ),
),
Expand All @@ -856,9 +844,6 @@ public function test_merge_incoming_data_null_presets() {
'color' => array(
'custom' => false,
),
'layout' => array(
'contentSize' => '610px',
),
'spacing' => array(
'customMargin' => false,
),
Expand Down Expand Up @@ -899,10 +884,6 @@ public function test_merge_incoming_data_null_presets() {
),
),
),
'layout' => array(
'contentSize' => '610px',
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what this test is about but this config is valid (contentSize) and maybe we could keep it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This commit is removing a previous commit in this PR: I had added this when I thought layout.units was a valid input and wanted to make sure the merge algorithm respected pre-existing data (layout.contentSize). If we want a test for this it should live in another method of this file. Can be done in a follow-up PR if necessary.

'units' => array( 'px', 'em' ),
),
'spacing' => array(
'customMargin' => false,
'units' => array( 'px', 'em' ),
Expand Down