Skip to content

Conversation

@yansern
Copy link
Contributor

@yansern yansern commented Aug 6, 2020

Description

This PR fixes min height input stuck on single digit after entering a value and clearing it using backspace.

The reason this is happening is because the onChange input fires the last known valid value when the input field is empty. The onChange event by UnitControl is actually a modified version of the onChange event which has a different behaviour than the native onChange event.

The solution is to use onInput which ensures the event handler always executes when input value has changed.

More detail of this issue here: Automattic/wp-calypso#43961

How has this been tested?

  1. Create a cover block.
  2. Enter a value, e.g. 400.
  3. Press backspace until input is cleared.
  4. Enter any single digit value (0-9), you'll always get 4.

Screenshots

See gif video at Automattic/wp-calypso#43961

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

The reason this is happening is because the onChange input fires the last known valid value when the input field is empty. The onChange event by UnitControl is actually a modified version of the onChange event which has a different behaviour than the native onChange event.

The solution is to use onInput which ensures the event handler always executes when input value has changed.

AFAICS, the Cover block is the only block that currently uses the UnitControl component, so I think this behavior of its onChange event should be deemed a bug in the component that we need to fix there (rather than work around it) 🤔

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

cc/ @ItsJonQ

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

The issue is probably somehwere in here:

const handleOnChange = ( next, changeProps ) => {
/**
* Customizing the onChange callback.
* This allows as to broadcast a combined value+unit to onChange.
*/
const [ parsedValue, parsedUnit ] = getValidParsedUnit(
next,
units,
value,
unit
);
const nextValue = `${ parsedValue }${ parsedUnit }`;
onChange( nextValue, changeProps );
};

(Also see the component's README for some information on how it's supposed to work.)

@ockham
Copy link
Contributor

ockham commented Aug 6, 2020

Note that the component has unit tests -- ideally we add something to cover this behavior.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

(I suggest fixing the UnitControl component instead.)

@ockham
Copy link
Contributor

ockham commented Aug 18, 2020

Superseded by #24490 😅

@ockham ockham closed this Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants