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

Reuse cleanEmptyObject utility and fix empty string case #49750

Merged
merged 6 commits into from
Apr 24, 2023
Merged

Reuse cleanEmptyObject utility and fix empty string case #49750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78798c6
Reuse cleanEmptyObject utility and fix empty string case
youknowriad Apr 12, 2023
dc3cdee
Clean falsy values
youknowriad Apr 18, 2023
eab5c90
Small simplification.
youknowriad Apr 19, 2023
50350ae
Add more unit tests to the clean function
youknowriad Apr 20, 2023
f154e5c
Clarify comment
youknowriad Apr 20, 2023
fe13b8f
Fix linting
youknowriad Apr 20, 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
16 changes: 13 additions & 3 deletions packages/block-editor/src/components/global-styles/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { useCallback, Platform } from '@wordpress/element';
/**
* Internal dependencies
*/
import { getValueFromVariable } from './utils';
import { getValueFromVariable, normalizeFalsyValue } from './utils';
import SpacingSizesControl from '../spacing-sizes-control';
import HeightControl from '../height-control';
import ChildLayoutControl from '../child-layout-control';
Expand Down Expand Up @@ -226,7 +226,11 @@ export default function DimensionsPanel( {
const contentSizeValue = decodeValue( inheritedValue?.layout?.contentSize );
const setContentSizeValue = ( newValue ) => {
onChange(
immutableSet( value, [ 'layout', 'contentSize' ], newValue )
immutableSet(
value,
[ 'layout', 'contentSize' ],
normalizeFalsyValue( newValue )
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm undecided between this and something explicit and immediate like newValue || undefined. While I understand the value of having a function like normalizeFalsyValue acting as an "anchor" (complete with documentation and the rationale for normalisation), I'm not sure that function name is descriptive enough.

That said, you could say I am picking nits; I'm wondering about how to make all the hidden traps of Global Styles as evident as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it's not a perfect solution, my main motivation is the documentation actually but I'm also undecided, especially since IMO we should be moving away slowly from its usage.

Copy link
Member

Choose a reason for hiding this comment

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

I think the undefined solution makes a lot of sense 👍

However, I personally think that:

  • There are far too many || undefined to generalize this as a usable abstraction, and while here we're starting to utilize ``normalizeFalsyValue` in a few places, there are still way more that will remain as they are.
  • || undefined is far too straightforward and simple to be generalized, and I think is more readable as-is instead of going through normalizeFalsyValue()

That being said, I'd personally lean towards inlining || undefined everywhere instead of introducing a function to do it. I think having it inline is more readable and comprehensible than abstracting it in a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I merged and missed this comment. I can remove that function soon if you feel strongly about it. I'm not sure how to handle the loss of documentation thought without a centralized function.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't mind, I'd prefer removing it, yes. I'm not sure there's a good need for documentation on this part, really. || undefined seems quite straightforward as a JS idiom and even if we abstract it to a function with docs (like we did), this won't help ensure that folks are using it where necessary. I'm also again pointing to the tens of usages of || undefined in the codebase where the usage isn't documented but is still there intentionally. Maybe I just fail to understand what you consider miscommunicated with the lack of documentation in these instances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I wanted to communicate is that for styles UI (most of them), clearing inputs means reset to the theme.json values. So it's not just a random JS operation to replace empty strings with undefined, it has bigger impacts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened this #50033

Copy link
Member

Choose a reason for hiding this comment

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

Is this something we should document at a different place then - for example in the primary README of the global styles component in the block editor? I'm just not sure if documenting it within a separate helper function will help much with adopting it, especially when folks aren't aware that they should be using that function.

)
);
};
const hasUserSetContentSizeValue = () => !! value?.layout?.contentSize;
Expand All @@ -237,7 +241,13 @@ export default function DimensionsPanel( {
useHasWideSize( settings ) && includeLayoutControls;
const wideSizeValue = decodeValue( inheritedValue?.layout?.wideSize );
const setWideSizeValue = ( newValue ) => {
onChange( immutableSet( value, [ 'layout', 'wideSize' ], newValue ) );
onChange(
immutableSet(
value,
[ 'layout', 'wideSize' ],
normalizeFalsyValue( newValue )
)
);
};
const hasUserSetWideSizeValue = () => !! value?.layout?.wideSize;
const resetWideSizeValue = () => setWideSizeValue( undefined );
Expand Down
46 changes: 36 additions & 10 deletions packages/block-editor/src/components/global-styles/typography-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import LineHeightControl from '../line-height-control';
import LetterSpacingControl from '../letter-spacing-control';
import TextTransformControl from '../text-transform-control';
import TextDecorationControl from '../text-decoration-control';
import { getValueFromVariable } from './utils';
import { getValueFromVariable, normalizeFalsyValue } from './utils';
import { immutableSet } from '../../utils/object';

const MIN_TEXT_COLUMNS = 1;
Expand Down Expand Up @@ -166,7 +166,9 @@ export default function TypographyPanel( {
immutableSet(
value,
[ 'typography', 'fontFamily' ],
slug ? `var:preset|font-family|${ slug }` : newValue
slug
? `var:preset|font-family|${ slug }`
: normalizeFalsyValue( newValue )
)
);
};
Expand All @@ -188,7 +190,11 @@ export default function TypographyPanel( {
: newValue;

onChange(
immutableSet( value, [ 'typography', 'fontSize' ], actualValue )
immutableSet(
value,
[ 'typography', 'fontSize' ],
normalizeFalsyValue( actualValue )
)
);
};
const hasFontSize = () => !! value?.typography?.fontSize;
Expand All @@ -209,8 +215,8 @@ export default function TypographyPanel( {
...value,
typography: {
...value?.typography,
fontStyle: newFontStyle,
fontWeight: newFontWeight,
fontStyle: normalizeFalsyValue( newFontStyle ),
fontWeight: normalizeFalsyValue( newFontWeight ),
},
} );
};
Expand All @@ -225,7 +231,11 @@ export default function TypographyPanel( {
const lineHeight = decodeValue( inheritedValue?.typography?.lineHeight );
const setLineHeight = ( newValue ) => {
onChange(
immutableSet( value, [ 'typography', 'lineHeight' ], newValue )
immutableSet(
value,
[ 'typography', 'lineHeight' ],
normalizeFalsyValue( newValue )
)
);
};
const hasLineHeight = () => !! value?.typography?.lineHeight;
Expand All @@ -238,7 +248,11 @@ export default function TypographyPanel( {
);
const setLetterSpacing = ( newValue ) => {
onChange(
immutableSet( value, [ 'typography', 'letterSpacing' ], newValue )
immutableSet(
value,
[ 'typography', 'letterSpacing' ],
normalizeFalsyValue( newValue )
)
);
};
const hasLetterSpacing = () => !! value?.typography?.letterSpacing;
Expand All @@ -249,7 +263,11 @@ export default function TypographyPanel( {
const textColumns = decodeValue( inheritedValue?.typography?.textColumns );
const setTextColumns = ( newValue ) => {
onChange(
immutableSet( value, [ 'typography', 'textColumns' ], newValue )
immutableSet(
value,
[ 'typography', 'textColumns' ],
normalizeFalsyValue( newValue )
)
);
};
const hasTextColumns = () => !! value?.typography?.textColumns;
Expand All @@ -262,7 +280,11 @@ export default function TypographyPanel( {
);
const setTextTransform = ( newValue ) => {
onChange(
immutableSet( value, [ 'typography', 'textTransform' ], newValue )
immutableSet(
value,
[ 'typography', 'textTransform' ],
normalizeFalsyValue( newValue )
)
);
};
const hasTextTransform = () => !! value?.typography?.textTransform;
Expand All @@ -275,7 +297,11 @@ export default function TypographyPanel( {
);
const setTextDecoration = ( newValue ) => {
onChange(
immutableSet( value, [ 'typography', 'textDecoration' ], newValue )
immutableSet(
value,
[ 'typography', 'textDecoration' ],
normalizeFalsyValue( newValue )
)
);
};
const hasTextDecoration = () => !! value?.typography?.textDecoration;
Expand Down
13 changes: 13 additions & 0 deletions packages/block-editor/src/components/global-styles/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,3 +376,16 @@ export function scopeSelector( scope, selector ) {

return selectorsScoped.join( ', ' );
}

/**
* Some Global Styles UI components (font size for instance) relies on the fact that emptying inputs
* (empty strings) should fallback to the parent value (theme.json value).
* Ideally, there should be a dedicated UI element to "revert to theme" for each input instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm probably missing something, but isn't that what the "reset" buttons in the tools panel dropdowns are meant to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

The reset functionality of the ToolsPanel menu is intended to facilitate setting any given value back to undefined allowing it to fall back to the global style or theme value. The reset callback triggered by the panel's menu items, could really set it to any value though.

relies on the fact that emptying inputs (empty strings) should fallback to the parent value

I think this part of the comment is referring to when a control can be manually cleared by a user e.g. deleting the value within an input control. In this case, it is retained by the new cleanEmptyObject but for the global styles to fall back to the theme's value correctly, we need to ensure a user value isn't set.

Either way, I could be misunderstanding. Perhaps we could add some further clarity to this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I meant that each "style" should have its own dedicated "reset" link, similar to the global one. and I agree that the comment can be made clearer.

* But until we do, this function is used to transform falsy values to undefined values for these components
* which allows the global styles merge algorithm to revert to the theme.json value.
*
* @param {*} value Value to normalize.
*
* @return {undefined|*} normalized value.
*/
export const normalizeFalsyValue = ( value ) => value || undefined;
113 changes: 113 additions & 0 deletions packages/block-editor/src/hooks/test/anchor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/**
* WordPress dependencies
*/
import { applyFilters } from '@wordpress/hooks';

/**
* Internal dependencies
*/
import '../anchor';

const noop = () => {};

describe( 'anchor', () => {
const blockSettings = {
save: noop,
category: 'text',
title: 'block title',
};

describe( 'addAttribute()', () => {
const registerBlockType = applyFilters.bind(
null,
'blocks.registerBlockType'
);

it( 'should do nothing if the block settings do not define anchor support', () => {
const settings = registerBlockType( blockSettings );

expect( settings.attributes ).toBe( undefined );
} );

it( 'should assign a new anchor attribute', () => {
const settings = registerBlockType( {
...blockSettings,
supports: {
anchor: true,
},
} );

expect( settings.attributes ).toHaveProperty( 'anchor' );
} );

it( 'should not override attributes defined in settings', () => {
const settings = registerBlockType( {
...blockSettings,
supports: {
anchor: true,
},
attributes: {
anchor: {
type: 'string',
default: 'testAnchor',
},
},
} );

expect( settings.attributes.anchor ).toEqual( {
type: 'string',
default: 'testAnchor',
} );
} );
} );

describe( 'addSaveProps', () => {
const getSaveContentExtraProps = applyFilters.bind(
null,
'blocks.getSaveContent.extraProps'
);

it( 'should do nothing if the block settings do not define anchor support', () => {
const attributes = { anchor: 'foo' };
const extraProps = getSaveContentExtraProps(
{},
blockSettings,
attributes
);

expect( extraProps ).not.toHaveProperty( 'id' );
} );

it( 'should inject anchor attribute ID', () => {
const attributes = { anchor: 'foo' };
const extraProps = getSaveContentExtraProps(
{},
{
...blockSettings,
supports: {
anchor: true,
},
},
attributes
);

expect( extraProps.id ).toBe( 'foo' );
} );

it( 'should remove an anchor attribute ID when field is cleared', () => {
const attributes = { anchor: '' };
const extraProps = getSaveContentExtraProps(
{},
{
...blockSettings,
supports: {
anchor: true,
},
},
attributes
);

expect( extraProps.id ).toBe( null );
} );
} );
} );
9 changes: 0 additions & 9 deletions packages/block-editor/src/hooks/test/color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,8 @@ import { registerBlockType, unregisterBlockType } from '@wordpress/blocks';
* Internal dependencies
*/
import BlockEditorProvider from '../../components/provider';
import { cleanEmptyObject } from '../utils';
import { withColorPaletteStyles } from '../color';

describe( 'cleanEmptyObject', () => {
it( 'should remove nested keys', () => {
expect( cleanEmptyObject( { color: { text: undefined } } ) ).toEqual(
undefined
);
} );
} );

describe( 'withColorPaletteStyles', () => {
const settings = {
__experimentalFeatures: {
Expand Down
Loading