Shadow Panel: Generates unique shadow slugs by finding max suffix and incrementing it

[amitraj2203]

What?

Fixes: #61989

Why?

Shadow slugs were getting duplicated when we try to delete any custom shadow and add a new. See this for more details.

How?

This PR adds a function to generate unique names and slugs for shadows by finding the highest suffix number used and incrementing it.

Testing Instructions

1. Add three custom shadows. The labels are "Shadow 1", "Shadow 2", and "Shadow3" respectively.
2. Delete "Shadow 2".
3. Add a new shadow.
4. Now the new shadow will have unique slug and name.

Screenshots or screencast

Screen.Recording.2024-05-26.at.10.23.54.PM.mov

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: amitraj2203 <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: talldan <[email protected]>
Co-authored-by: vk17-starlord <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[t-hamano]

Thanks for the PR!

I think the approach makes sense, but personally I think this function needs to be made more generic. Similar problems will be faced when the UI for customizing font size presets and spacing scale values are implemented.

I haven't tested it, but what about a function that simply returns just the index, like below? This allows us to apply it to other presets in the future.

export function getNewIndexFromPresets( presets, slugPrefix ) {
	const nameRegex = new RegExp( `^${ slugPrefix }([\\d]+)$` );
	return presets.reduce( ( previousValue, currentValue ) => {
		if ( typeof currentValue?.slug === 'string' ) {
			const matches = currentValue?.slug.match( nameRegex );
			if ( matches ) {
				const id = parseInt( matches[ 1 ], 10 );
				if ( id >= previousValue ) {
					return id + 1;
				}
			}
		}
		return previousValue;
	}, 1 );
}

const handleAddShadow = () => {
	const newIndex = getNewIndexFromPresets( shadows, 'shadow-' );
	onCreate( {
		name: sprintf(
			/* translators: %s: is an index for a preset */
			__( 'Shadow %s' ),
			newIndex
		),
		slug: `shadow-${ newIndex }`,
		shadow: defaultShadow,
	} );
};

Additionally, we may want to consider:

• Considering the possibility that this function will be used for general purposes in the future, move the function to packages/edit-site/src/components/global-styles/utils.js.
• Add JSDoc to the function
• Add some unit tests to packages/edit-site/src/components/global-styles/test

[amitraj2203]

Hi @t-hamano, as suggested I have created a generic function for getting the Index and added some unit tests. Let me know if there are any further improvements you'd like to make.

[t-hamano]

@amitraj2203

Thanks for the update!

@madhusudhand @jorgefilipecosta What do you think about this approach? I'm sure you've implemented shadow UI, so I'd love to hear your advice 🙇‍♂️

[talldan]

The logic in the function looks good to me - find the highest preset and increment it by one, nice work putting that together and adding the associated tests.

I think the docblock could be a little more descriptive about what the function does. It mentions 'new index', but the reader has no idea what kind of value that new index will be. It should be possible to understand exactly what the function returns without reading any of the code.

[amitraj2203]

Acknowledged! I've updated the docblock to provide a clearer description of the function's purpose and its return value.

[talldan]

Minor optional feedback (it doesn't need to be addressed), but I think the function can be more self documenting with some small changes to the variable naming and if the increment happens at the end:

	const highestPresetValue = presets.reduce( ( currentHighest, preset ) => {
		if ( typeof preset?.slug === 'string' ) {
			const matches = preset?.slug.match( nameRegex );
			if ( matches ) {
				const id = parseInt( matches[ 1 ], 10 );
				if ( id > currentHighest ) {
					return id;
				}
			}
		}
		return currentHighest;
	}, 0 );
	return highestPresetValue + 1;

I haven't tested it though, so not sure if this code still works 😄

[amitraj2203]

Thanks for the review. I completely agree with your suggestion and also I've tested these changes and it is working as expected.

[t-hamano]

LGTM! I think it's working as expected.

c42ca7267e953f15aba0486d6039eaa7.mp4

Could you rebase this PR with the latest trunk? Then the failing GitHub Actions should pass.

I would be happy if you could give us a second re-review, @talldan.

[talldan]

Looks good to me too, thanks for your contribution 🎉

I'll go ahead and merge the PR, as I don't think it will have had an impact on bundle size.