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

Alpha-enabled color-picker component doesn't run onChangeComplete when changed from the form #26568

Closed
Tracked by #34284
abaicus opened this issue Oct 29, 2020 · 2 comments
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@abaicus
Copy link
Contributor

abaicus commented Oct 29, 2020

Describe the bug
Alpha-enabled color-picker doesn't always run onChangeComplete function.

To reproduce
Steps to reproduce the behavior:

  1. Go to the storybook for the Alpha-enabled Color Picker or insert a ColorPicker component inside React with alpha enabled.
  2. Click on the Change color format button until you land on any other form than the Hexadecimal one.
  3. If you change any of the values inside the form, the onChangeComplete function never runs.

Expected behavior:
onChangeComplete should be run when changing anything inside the form.

Editor version:

  • WordPress version: 5.5.1
  • Gutenberg Plugin
  • Version 9.2.2 - Latest from the master branch on this repository

Desktop:

  • OS: Ubuntu
  • Browser Chrome
  • Version 86.0.4240.75
@abaicus abaicus changed the title Alpha-enabled color-picker component doesn't run onChangeComplete when changed the form Alpha-enabled color-picker component doesn't run onChangeComplete when changed from the form Oct 29, 2020
@talldan talldan added [Package] Components /packages/components Needs Testing Needs further testing to be confirmed. labels Nov 3, 2020
@talldan
Copy link
Contributor

talldan commented Nov 3, 2020

From a quick look, the way debounce is called for onChangeComplete doesn't look right:

commitValues( data ) {
const { oldHue, onChangeComplete = noop } = this.props;
if ( isValidColor( data ) ) {
const colors = colorToState( data, data.h || oldHue );
this.setState(
{
...colors,
draftHex: toLowerCase( colors.hex ),
draftHsl: colors.hsl,
draftRgb: colors.rgb,
},
debounce( partial( onChangeComplete, colors ), 100 )
);
}
}

Every time commitValues is called a new debounced function is created.

Instead the debounced function should be created in a constructor (if onChangeComplete is defined) and also in a lifecycle function like componentDidUpdate if the onChangeComplete callback changes.

Might be prudent to refactor it to a function component though and take advantage of effects.

@talldan talldan added [Type] Bug An existing feature does not function as intended and removed Needs Testing Needs further testing to be confirmed. labels Mar 30, 2021
@ciampo ciampo mentioned this issue Aug 25, 2021
31 tasks
@mirka
Copy link
Member

mirka commented Dec 2, 2021

This should be fixed now, as seen in the current Storybook.

@mirka mirka closed this as completed Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants