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

Make user able to change all color palette origins #36674

Merged
merged 3 commits into from Nov 22, 2021
Merged

Make user able to change all color palette origins #36674

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb6d2fa
Make user able to change all color palette origins
jorgefilipecosta Nov 19, 2021
625e654
Feedback
jorgefilipecosta Nov 22, 2021
2553294
Fix PHP lint issues
oandregal Nov 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 35 additions & 31 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Expand Up @@ -301,7 +301,9 @@ public function __construct( $theme_json = array(), $origin = 'theme' ) {
$path = array_merge( $node['path'], $preset_metadata['path'] );
$preset = _wp_array_get( $this->theme_json, $path, null );
if ( null !== $preset ) {
_wp_array_set( $this->theme_json, $path, array( $origin => $preset ) );
if ( 'user' !== $origin || isset( $preset[0] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd think we want to rename the user origin to custom, as we did with core to default to match the UI, class/css var generation, etc. Perhaps we can do this in a different PR that lands before this one?

Copy link
Member

@oandregal oandregal Nov 22, 2021 •
edited

Choose a reason for hiding this comment

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

What's the use case isset( $preset[ 0 ] ) is checking for? My understanding is that we should always skip reorganizing the presets if the origin is the user, as we'll store them already keyed by origin, so this could be simplified to: if ( null != $preset && 'user' !== $origin ).

Additionally, I wonder if we need to keep back compatibility with existing user data in the database. We could add an extra step to normalize the user data by origin for that case (older data that is not keyed by origin).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see now: the isset( $preset[ 0 ] ) already does the back-compatibility trick :) Only old user data with no keys by origin would have the 0 key.

_wp_array_set( $this->theme_json, $path, array( $origin => $preset ) );
}
}
}
}
Expand Down Expand Up @@ -1335,46 +1337,48 @@ public function merge( $incoming ) {
private static function remove_insecure_settings( $input ) {
$output = array();
foreach ( self::PRESETS_METADATA as $preset_metadata ) {
$presets = _wp_array_get( $input, $preset_metadata['path'], null );
if ( null === $presets ) {
continue;
}
foreach( self::VALID_ORIGINS as $origin ) {
$path_with_origin = array_merge( $preset_metadata['path'], array( $origin ) );
$presets = _wp_array_get( $input, $path_with_origin, null );
if ( null === $presets ) {
continue;
}

$escaped_preset = array();
foreach ( $presets as $preset ) {
if (
esc_attr( esc_html( $preset['name'] ) ) === $preset['name'] &&
sanitize_html_class( $preset['slug'] ) === $preset['slug']
) {
$value = null;
if ( isset( $preset_metadata['value_key'] ) ) {
$value = $preset[ $preset_metadata['value_key'] ];
} elseif (
isset( $preset_metadata['value_func'] ) &&
is_callable( $preset_metadata['value_func'] )
$escaped_preset = array();
foreach ( $presets as $preset ) {
if (
esc_attr( esc_html( $preset['name'] ) ) === $preset['name'] &&
sanitize_html_class( $preset['slug'] ) === $preset['slug']
) {
$value = call_user_func( $preset_metadata['value_func'], $preset );
}
$value = null;
if ( isset( $preset_metadata['value_key'] ) ) {
$value = $preset[ $preset_metadata['value_key'] ];
} elseif (
isset( $preset_metadata['value_func'] ) &&
is_callable( $preset_metadata['value_func'] )
) {
$value = call_user_func( $preset_metadata['value_func'], $preset );
}

$preset_is_valid = true;
foreach ( $preset_metadata['properties'] as $property ) {
if ( ! self::is_safe_css_declaration( $property, $value ) ) {
$preset_is_valid = false;
break;
$preset_is_valid = true;
foreach ( $preset_metadata['properties'] as $property ) {
if ( ! self::is_safe_css_declaration( $property, $value ) ) {
$preset_is_valid = false;
break;
}
}
}

if ( $preset_is_valid ) {
$escaped_preset[] = $preset;
if ( $preset_is_valid ) {
$escaped_preset[] = $preset;
}
}
}
}

if ( ! empty( $escaped_preset ) ) {
_wp_array_set( $output, $preset_metadata['path'], $escaped_preset );
if ( ! empty( $escaped_preset ) ) {
_wp_array_set( $output, $path_with_origin, $escaped_preset );
}
}
}

return $output;
}

Expand Down
111 changes: 70 additions & 41 deletions packages/components/src/color-edit/index.js
View file
Open in desktop
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import kebabCase from 'lodash';
import { kebabCase } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -49,12 +49,14 @@ function ColorNameInput( { value, onChange } ) {
}

function ColorOption( {
canOnlyChangeValues,
color,
onChange,
isEditing,
onStartEditing,
onRemove,
onStopEditing,
slugPrefix,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very keen on making the ColorEdit component aware of the prefix for custom colors. For components, do we usually expose "hooks" for consumers to tweak the normal behavior? Like, for example, we could have an onNameChange callback that consumers can use to tweak the name as they like (add a prefix, etc).

Not really familiar with how/whether this is a practice we want our components to have, so I'll go with whatever you think is best.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the prefix may end up being used for other things besides custom colors, and it seems we will probably not need other name manipulation than the prefix. So I guess slugPrefix is enough. In the future, I'm not totally sure of the API and the place where this component will be. I guess when we remove experimental from this component or move the component to another package we should audit its API.

} ) {
const focusOutsideProps = useFocusOutside( onStopEditing );
return (
Expand All @@ -68,22 +70,22 @@ function ColorOption( {
<ColorIndicatorStyled colorValue={ color.color } />
</FlexItem>
<FlexItem>
{ isEditing ? (
{ isEditing && ! canOnlyChangeValues ? (
<ColorNameInput
value={ color.name }
onChange={ ( nextName ) =>
onChange( {
...color,
name: nextName,
slug: kebabCase( nextName ),
slug: slugPrefix + kebabCase( nextName ),
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. We need to fix the kebabCase import to import { kebabCase } from 'lodash';.

} )
}
/>
) : (
<ColorNameContainer>{ color.name }</ColorNameContainer>
) }
</FlexItem>
{ isEditing && (
{ isEditing && ! canOnlyChangeValues && (
<FlexItem>
<RemoveButton
isSmall
Expand Down Expand Up @@ -119,6 +121,8 @@ function ColorPaletteEditListView( {
onChange,
editingColor,
setEditingColor,
canOnlyChangeValues,
slugPrefix,
} ) {
// When unmounting the component if there are empty colors (the user did not complete the insertion) clean them.
const colorReference = useRef();
Expand All @@ -140,6 +144,7 @@ function ColorPaletteEditListView( {
<ItemGroup isBordered isSeparated>
{ colors.map( ( color, index ) => (
<ColorOption
canOnlyChangeValues={ canOnlyChangeValues }
key={ index }
color={ color }
onStartEditing={ () => {
Expand Down Expand Up @@ -177,6 +182,7 @@ function ColorPaletteEditListView( {
setEditingColor( null );
}
} }
slugPrefix={ slugPrefix }
/>
) ) }
</ItemGroup>
Expand All @@ -186,7 +192,15 @@ function ColorPaletteEditListView( {

const EMPTY_ARRAY = [];

export default function ColorEdit( { colors = EMPTY_ARRAY, onChange } ) {
export default function ColorEdit( {
colors = EMPTY_ARRAY,
onChange,
paletteLabel,
emptyMessage,
canOnlyChangeValues,
canReset,
slugPrefix = '',
} ) {
const [ isEditing, setIsEditing ] = useState( false );
const [ editingColor, setEditingColor ] = useState( null );
const isAdding =
Expand All @@ -200,7 +214,7 @@ export default function ColorEdit( { colors = EMPTY_ARRAY, onChange } ) {
return (
<ColorEditStyles>
<ColorHStackHeader>
<ColorHeading>{ __( 'Custom' ) }</ColorHeading>
<ColorHeading>{ paletteLabel }</ColorHeading>
<ColorActionsContainer>
{ isEditing && (
<DoneButton
Expand All @@ -213,24 +227,26 @@ export default function ColorEdit( { colors = EMPTY_ARRAY, onChange } ) {
{ __( 'Done' ) }
</DoneButton>
) }
<Button
isSmall
isPressed={ isAdding }
icon={ plus }
label={ __( 'Add custom color' ) }
onClick={ () => {
onChange( [
...colors,
{
color: '#000',
name: '',
slug: '',
},
] );
setIsEditing( true );
setEditingColor( colors.length );
} }
/>
{ ! canOnlyChangeValues && (
<Button
isSmall
isPressed={ isAdding }
icon={ plus }
label={ __( 'Add color' ) }
onClick={ () => {
onChange( [
...colors,
{
color: '#000',
name: '',
slug: '',
},
] );
setIsEditing( true );
setEditingColor( colors.length );
} }
/>
) }
{ ! isEditing && (
<Button
disabled={ ! hasColors }
Expand All @@ -242,28 +258,42 @@ export default function ColorEdit( { colors = EMPTY_ARRAY, onChange } ) {
} }
/>
) }
{ isEditing && (
{ isEditing && ( canReset || ! canOnlyChangeValues ) && (
<DropdownMenu
icon={ moreVertical }
label={ __( 'Custom color options' ) }
label={ __( 'Color options' ) }
toggleProps={ {
isSmall: true,
} }
>
{ ( { onClose } ) => (
<>
<NavigableMenu role="menu">
<Button
variant="tertiary"
onClick={ () => {
setEditingColor( null );
setIsEditing( false );
onChange();
onClose();
} }
>
{ __( 'Remove all custom colors' ) }
</Button>
{ ! canOnlyChangeValues && (
<Button
variant="tertiary"
onClick={ () => {
setEditingColor( null );
setIsEditing( false );
onChange();
onClose();
} }
>
{ __( 'Remove all colors' ) }
</Button>
) }
{ canReset && (
<Button
variant="tertiary"
onClick={ () => {
setEditingColor( null );
onChange();
onClose();
} }
>
{ __( 'Reset colors' ) }
</Button>
) }
</NavigableMenu>
</>
) }
Expand All @@ -275,10 +305,12 @@ export default function ColorEdit( { colors = EMPTY_ARRAY, onChange } ) {
<>
{ isEditing && (
<ColorPaletteEditListView
canOnlyChangeValues={ canOnlyChangeValues }
colors={ colors }
onChange={ onChange }
editingColor={ editingColor }
setEditingColor={ setEditingColor }
slugPrefix={ slugPrefix }
/>
) }
{ ! isEditing && (
Expand All @@ -291,10 +323,7 @@ export default function ColorEdit( { colors = EMPTY_ARRAY, onChange } ) {
) }
</>
) }
{ ! hasColors &&
__(
'Custom colors are empty! Add some colors to create your own color palette.'
) }
{ ! hasColors && emptyMessage }
</ColorEditStyles>
);
}