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.

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

onDeselect not supported in ColorGradientSettingsDropdown component #60536

Open
permafrost06 opened this issue Apr 6, 2024 · 3 comments
Open
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended

Comments

@permafrost06
Copy link

permafrost06 commented Apr 6, 2024

Description

Even though ColorGradientSettingsDropdown returns a ToolsPanelItem (which supports the onDeselect property), the onDeselect property is not supported by ColorGradientSettingsDropdown. Instead, when a single setting is reset, the value of the associated attribute is set to undefined which is not always the expected behaviour. E.g, the default color may be something else.

Step-by-step reproduction instructions

  1. Import ColorGradentSettingsDropdown from package @wordpress/block-editor:
import { __experimentalColorGradientSettingsDropdown as ColorGradientSettingsDropdown } from  "@wordpress/block-editor";
  1. Set up the component inside InspectorControls. For example, this is how I'm using the ColorGradentSettingsDropdown component:
<InspectorControls group="color">
    <ColorGradientSettingsDropdown
        enableAlpha
        panelId={clientId}
        title={__("Color Settings", "tableberg")}
        popoverProps={{
            placement: "left start",
        }}
        settings={[{
            label: "Star Color",
            colorValue: starColor,
            onColorChange: (starColor) => setAttributes({ starColor }),
            clearable: true,
            onDeselect: () => setAttributes({ starColor: "#FFB901" }),
            resetAllFilter: () => setAttributes({ starColor: "#FFB901" }),
        }]}
    />
</InspectorControls>
  1. Check that in the rendered setting, when the "Reset all" button is clicked, the supplied resetAllFilter function is executed - attribute starColor is set to "#FFB901". However, the onDeselect prop isn't working as expected. Instead of setting attribute starColor to "#FFB901", starColor is set to undefined. This is demonstrated in the video below.

Screenshots, screen recording, code snippet

gutenberg-color-dropdown-issue.mp4

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@permafrost06 permafrost06 added the [Type] Bug An existing feature does not function as intended label Apr 6, 2024
@permafrost06
Copy link
Author

This happens because in packages/block-editor/src/components/colors-gradients/dropdown.js line 40, the onDeselect prop is bound to the clearValue function. This function sets the color attribute to undefined. I have submitted a PR #60487 with a fix for this.

@carolinan
Copy link
Contributor

I am still trying to understand what the end goal is here and I am not sure that updating the default behaviour of a single component is the correct solution.

I believe you may be thinking of these feature requests about showing the default values in the controls:

#37752
#57296
#43082

@permafrost06
Copy link
Author

@carolinan sorry for taking up so much of your time for what is admittedly a small problem. My primary language isn't English, so I sometimes struggle to find words and cannot easily express what I want to say. Thank you for being so helpful and patient with me over weeks 🥹.

I will try to give straightforward responses to your concerns.

  1. Clarifying my issue: When the ColorGradientSettingsDropdown component is used in the way I have described in my issue, resetting a single color and resetting all colors have different behaviours. Resetting all colors works as expected (sets star color to gold) but resetting the star color only sets the color to black. Even though I have set both onDeselect and resetAllFilter to set the star color to gold.
box-240424-0221-47.mp4
  1. Why the issue is occurring: The code in ColorGradientSettingsDropdown component makes an assumption that the default value of a color/gradient is undefined (it calls the value change function without any arguments which in turn is undefined, but I guess you get that). undefined is not always the default color as I've demonstrated in my issue. This is the code from ColorGradientSettingsDropdown (trimmed for brevity):
const WithToolsPanelItem = ( { setting, children, panelId, ...props } ) => {
	const clearValue = () => {
		if ( setting.colorValue ) {
			setting.onColorChange();
		} else if ( setting.gradientValue ) {
			setting.onGradientChange();
		}
	};
	return (
		<ToolsPanelItem
			/* other props */
			onDeselect={ clearValue }
			resetAllFilter={ setting.resetAllFilter }
		>
			{ children }
		</ToolsPanelItem>
	);
};

Notice that onDeselect is set to the clearValue function which calls the value change handler function (passed via the settings) without arguments i.e. sets the color to undefined. Whereas resetAllFilter (correctly) calls the function passed by the setting object. The code ignores the onDeselect in the setting object and this is the reason the problem is occurring.

  1. The end goal: To make clicking the "Reset all" button and the "Reset" button of a color to have the same (and intended) effect.
  2. Why updating the code only for this component fixes the issue: This is not a widespread problem. This pattern of using the clearValue function to reset a value seems to be unique to this ColorGradientSettingsDropdown component. Hence, the fix for this specific component.
  3. The issues you have referenced: The feature request issues you have referenced are about showing the computed values of the default option in various controls. The issue I'm facing is different from these. I don't care whether or not the color indicator shows gold or the 🚫 icon but I want my color to be correct when resetted. My issue is not a feature request at all. It is a bug report. Resetting the same thing via different buttons shouldn't produce two different results, right?

If you find a mistake in my code or what I've said is incorrect, please let me know and I'll close the issue and the associated PR. I do not wish to waste any more of your time. I apologize in advance if this is the case.

In case you're wondering, the default value for the star color attribute is set to gold (#FFB901) in my block.json.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants