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

Palette Edit: Don't discards colors with default name and slug #54332

Merged
merged 7 commits into from
Dec 22, 2023
Merged

Palette Edit: Don't discards colors with default name and slug #54332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
729f70e
Palette Edit: Don't discards colors with default name and slug
t-hamano Sep 10, 2023
eb125e7
Update comments and variables
t-hamano Sep 10, 2023
1547507
Update changelog
t-hamano Sep 10, 2023
54b2e9a
Merge branch 'trunk' into palette-edit/keep-default-palette
t-hamano Dec 20, 2023
7f3524e
Remove isTemporaryElement function and tests
t-hamano Dec 20, 2023
cb9e94a
Fix changelog typo
t-hamano Dec 21, 2023
676611a
Merge branch 'trunk' into palette-edit/keep-default-palette
t-hamano Dec 22, 2023
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- `FormTokenField`: Fix a regression where the suggestion list would re-open after clicking away from the input ([#57002](https://github.com/WordPress/gutenberg/pull/57002)).
- `Snackbar`: Remove erroneous `__unstableHTML` prop from TypeScript definitions ([#57218](https://github.com/WordPress/gutenberg/pull/57218)).
- `Truncate`: improve handling of non-string `children` ([#57261](https://github.com/WordPress/gutenberg/pull/57261)).
- `PaletteEdit`: Don't discard colors with default name and slug ([#54332](https://github.com/WordPress/gutenberg/pull/54332)).

### Enhancements

Expand Down
65 changes: 11 additions & 54 deletions packages/components/src/palette-edit/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import type {
PaletteElement,
} from './types';

export const DEFAULT_COLOR = '#000';
const DEFAULT_COLOR = '#000';

function NameInput( { value, onChange, label }: NameInputProps ) {
return (
Expand All @@ -74,8 +74,8 @@ function NameInput( { value, onChange, label }: NameInputProps ) {
}

/**
* Returns a temporary name for a palette item in the format "Color + id".
* To ensure there are no duplicate ids, this function checks all slugs for temporary names.
* Returns a name for a palette item in the format "Color + id".
* To ensure there are no duplicate ids, this function checks all slugs.
* It expects slugs to be in the format: slugPrefix + color- + number.
* It then sets the id component of the new name based on the incremented id of the highest existing slug id.
*
Expand All @@ -88,10 +88,10 @@ export function getNameForPosition(
elements: PaletteElement[],
slugPrefix: string
) {
const temporaryNameRegex = new RegExp( `^${ slugPrefix }color-([\\d]+)$` );
const nameRegex = new RegExp( `^${ slugPrefix }color-([\\d]+)$` );
const position = elements.reduce( ( previousValue, currentValue ) => {
if ( typeof currentValue?.slug === 'string' ) {
const matches = currentValue?.slug.match( temporaryNameRegex );
const matches = currentValue?.slug.match( nameRegex );
if ( matches ) {
const id = parseInt( matches[ 1 ], 10 );
if ( id >= previousValue ) {
Expand All @@ -103,7 +103,7 @@ export function getNameForPosition(
}, 1 );

return sprintf(
/* translators: %s: is a temporary id for a custom color */
/* translators: %s: is an id for a custom color */
__( 'Color %s' ),
position
);
Expand Down Expand Up @@ -261,32 +261,6 @@ function Option< T extends Color | Gradient >( {
);
}

/**
* Checks if a color or gradient is a temporary element by testing against default values.
*/
export function isTemporaryElement(
Copy link
Member

Choose a reason for hiding this comment

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

I actually think this is a decent approach to removing the complexity. Why do we remove "default" values? To keep the list clean? We could rather trust the user to control how they want to create custom colors/gradients.

The following doesn't make much sense, but why not?

2023-12-21.11.02.16.mp4

The only thing we could probably address here is the lack of default value on the first item of the gradient:

Screenshot 2023-12-21 at 11 03 18 am

It has an empty value and can be saved.

Copy link
Contributor

Choose a reason for hiding this comment

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

The following doesn't make much sense, but why not?

That's a good point... maybe the user knows they're going to add a few custom colors and quickly goes to create them and then decides to go back later to update their colors? It might be nice to allow these sorts of interactions rather than try to predict too much, so I'm really liking the direction of this PR.

slugPrefix: string,
{ slug, color, gradient }: Color | Gradient
): Boolean {
const regex = new RegExp( `^${ slugPrefix }color-([\\d]+)$` );

// If the slug matches the temporary name regex,
// check if the color or gradient matches the default value.
if ( regex.test( slug ) ) {
// The order is important as gradient elements
// contain a color property.
if ( !! gradient ) {
return gradient === DEFAULT_GRADIENT;
}

if ( !! color ) {
return color === DEFAULT_COLOR;
}
}

return false;
}

function PaletteEditListView< T extends Color | Gradient >( {
elements,
onChange,
Expand All @@ -302,23 +276,6 @@ function PaletteEditListView< T extends Color | Gradient >( {
useEffect( () => {
elementsReference.current = elements;
}, [ elements ] );
useEffect( () => {
return () => {
if (
elementsReference.current?.some( ( element ) =>
isTemporaryElement( slugPrefix, element )
)
) {
const newElements = elementsReference.current.filter(
( element ) => ! isTemporaryElement( slugPrefix, element )
);
onChange( newElements.length ? newElements : undefined );
}
};
// Disable reason: adding the missing dependency here would cause breaking changes that will require
// a heavier refactor to avoid. See https://github.com/WordPress/gutenberg/pull/43911
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [] );

const debounceOnChange = useDebounce( onChange, 100 );

Expand Down Expand Up @@ -474,7 +431,7 @@ export function PaletteEdit( {
: __( 'Add color' )
}
onClick={ () => {
const tempOptionName = getNameForPosition(
const optionName = getNameForPosition(
elements,
slugPrefix
);
Expand All @@ -484,21 +441,21 @@ export function PaletteEdit( {
...gradients,
{
gradient: DEFAULT_GRADIENT,
name: tempOptionName,
name: optionName,
slug:
slugPrefix +
kebabCase( tempOptionName ),
kebabCase( optionName ),
},
] );
} else {
onChange( [
...colors,
{
color: DEFAULT_COLOR,
name: tempOptionName,
name: optionName,
slug:
slugPrefix +
kebabCase( tempOptionName ),
kebabCase( optionName ),
},
] );
}
Expand Down
76 changes: 1 addition & 75 deletions packages/components/src/palette-edit/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,8 @@ import { render, fireEvent, screen } from '@testing-library/react';
/**
* Internal dependencies
*/
import PaletteEdit, {
getNameForPosition,
isTemporaryElement,
DEFAULT_COLOR,
} from '..';
import PaletteEdit, { getNameForPosition } from '..';
import type { PaletteElement } from '../types';
import { DEFAULT_GRADIENT } from '../../custom-gradient-picker/constants';

describe( 'getNameForPosition', () => {
test( 'should return 1 by default', () => {
Expand Down Expand Up @@ -85,75 +80,6 @@ describe( 'getNameForPosition', () => {
} );
} );

describe( 'isTemporaryElement', () => {
[
{
message: 'identifies temporary color',
slug: 'test-',
obj: {
name: '',
slug: 'test-color-1',
color: DEFAULT_COLOR,
},
expected: true,
},
{
message: 'identifies temporary gradient',
slug: 'test-',
obj: {
name: '',
slug: 'test-color-1',
gradient: DEFAULT_GRADIENT,
},
expected: true,
},
{
message: 'identifies custom color slug',
slug: 'test-',
obj: {
name: '',
slug: 'test-color-happy',
color: DEFAULT_COLOR,
},
expected: false,
},
{
message: 'identifies custom color value',
slug: 'test-',
obj: {
name: '',
slug: 'test-color-1',
color: '#ccc',
},
expected: false,
},
{
message: 'identifies custom gradient slug',
slug: 'test-',
obj: {
name: '',
slug: 'test-gradient-joy',
color: DEFAULT_GRADIENT,
},
expected: false,
},
{
message: 'identifies custom gradient value',
slug: 'test-',
obj: {
name: '',
slug: 'test-color-3',
color: 'linear-gradient(90deg, rgba(22, 22, 22, 1) 0%, rgb(155, 81, 224) 100%)',
},
expected: false,
},
].forEach( ( { message, slug, obj, expected } ) => {
it( `should ${ message }`, () => {
expect( isTemporaryElement( slug, obj ) ).toBe( expected );
} );
} );
} );

describe( 'PaletteEdit', () => {
const defaultProps = {
colors: [ { color: '#ffffff', name: 'Base', slug: 'base' } ],
Expand Down