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

Fix spacer NaN warning #14785

Merged
merged 5 commits into from Apr 15, 2019

Conversation

@Jackie6
Copy link
Contributor

commented Apr 3, 2019

Description

Fix #14426

How has this been tested?

  • local test
  • brower test

Types of changes

Change the value in BaseControl of the spacer block

Checklist:

  • [ X ] My code is tested.
  • [ X ] My code follows the WordPress code style.
  • [ X ] My code follows the accessibility standards.
  • [ X ] My code has proper inline documentation.
  • [ X ] I've included developer documentation if appropriate.
@talldan
Copy link
Contributor

left a comment

Hi again @Jackie6. This didn't seem to solve the issue when I tested it. My steps were:

  1. Add a spacer to post
  2. Delete the height value of the spacer in the sidebar
  3. Save and refresh the post

The issue seems to be that this code still causes NaN to be set as the attribute because parseInt is called with an empty string:

setAttributes( {
	height: parseInt( event.target.value, 10 ),
} );
@@ -58,7 +58,7 @@ const SpacerEdit = ( { attributes, isSelected, setAttributes, toggleSelection, i
height: parseInt( event.target.value, 10 ),
} );
} }
value={ height }
value={ isNaN( height ) ? '' : height }

This comment has been minimized.

Copy link
@kelin1003

kelin1003 Apr 5, 2019

Contributor

Instead of passing an empty string, you can pass 20 if height is NaN, since the min value of the input field is set to 20. ( check line 62 )

@Jackie6

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Hi @talldan Thanks for comments. What if we enter a value that is less than the min height?
image
Currently, we can set the height of the spacer to 2, but I think it should be at lest 20. Do we need to reset the value to 20 once we detect the input value is less than 20? Or the current warning is enough

@talldan

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

Hey @Jackie6. I think that current message is just provided by the browser, and not every browser shows it (I couldn't see it in Chrome).

I feel like the best behaviour might be to add some checks here:

setAttributes( {
height: parseInt( event.target.value, 10 ),
} );

  • If the size is less than 20, reset it to 20 as you suggested.
  • If the user deletes the value in the field, reset it to the default size of 100.

One thing to be careful about here is backward compatibility. It might be necessary to add a block deprecation:
https://wordpress.org/gutenberg/handbook/designers-developers/developers/block-api/block-deprecation/

@Jackie6

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Hi, @talldan, thanks for your suggestions. I modify the code as is discussed in your comment.

I find it is not very user-friendly. For example, if I would like to enter "40" as the height of the spacer,
YkbSIkvN3D
At first, I want to delete the default size of "100", but when I just delete the last zero, it becomes "20", and when I want to select the "20" and enter "40", once I enter a digit, it becomes "20" (because the number entered must be in the range of [0,9] ). So I have to append a zero first and replace the middle zero by four and finally delete the number two to create number "40". Or I have to use the range control and press the add button two times to create number "40" from the number "20".

As is showed in the code,

min="20"
step="10"

The valid height should be n*10, n is an integer at least 2. Why not just make the height uneditable? The height can only be adjusted by using the range control rather than enter a value directly.

In addition, I do not understand why a deprecation should be added. Because I do not include any new feature that may affect backward compatibility.

@talldan

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

Hey again @Jackie6. A solution could be to store the raw value of the input so that the user's changes to the input field aren't overwritten. Could use react's useState hook (which can be imported from @wordpress/element) for storing the input's value. I had a look to see if that would work and managed to get it working.

Something like:

const [ inputHeightValue, setInputHeightValue ] = useState( height );

// ...

<input
	onChange={ ( event ) => {
		let customHeight = parseInt( event.target.value, 10 );
		setInputHeightValue( customHeight );
		if ( isNaN( customHeight ) ) {
			// Set to the default size
			customHeight = 100;
		} else if ( customHeight < 20 ) {
			// Set to the minimum size
			customHeight = 20;
		}
		setAttributes( {
			height: customHeight,
		} );
	} }
	value={ inputHeightValue }

This way the value of the input field can be different to the displayed height of the spacer. Would also have to make sure to update the inputHeightValue when using the drag controls as well.

In addition, I do not understand why a deprecation should be added. Because I do not include any new feature that may affect backward compatibility.

You're right, sorry about that, there would need to be changes to the save function to consider adding a deprecation.

@Jackie6

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@talldan Thanks for comments. Now the code works better.
BTW, when the input box is empty, the spacer is set to the default size(i.e, 100). Although I think it makes senses, I am not sure if it is necessary to throw a warning.

image

@talldan

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

This is looking good, @Jackie6. One final thing I think needs to be done is to also update the input field's value when the drag handle is used on the spacer:

onResizeStop={ ( event, direction, elt, delta ) => {
	setAttributes( {
		height: parseInt( height + delta.height, 10 ),
	} );
	toggleSelection( true );
} }
@talldan
Copy link
Contributor

left a comment

Thanks for making those changes! 😄

@talldan talldan merged commit e167221 into WordPress:master Apr 15, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
@@ -15,6 +15,7 @@ import { withInstanceId } from '@wordpress/compose';
const SpacerEdit = ( { attributes, isSelected, setAttributes, toggleSelection, instanceId } ) => {
const { height } = attributes;
const id = `block-spacer-height-input-${ instanceId }`;
const [ inputHeightValue, setInputHeightValue ] = useState( height );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 15, 2019

Contributor

I'm not sure I understand why we need a sperate state value here. Why height is not enough?

This comment has been minimized.

Copy link
@talldan

talldan Apr 15, 2019

Contributor

When the user deletes the value in the input entirely, we wanted height to revert to the default of 100. That felt like the right user experience to me at the time.

It creates a situation where the input has one value '' and the spacer has another 100.

I was also thinking about an uncontrolled input, but then there's still a requirement to set the initial value.

Are there other ways to achieve this?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 15, 2019

Contributor

makes sense, I didn't see this subtle difference at first. Thanks for clarifying.

aduth added a commit that referenced this pull request Apr 15, 2019

Fix spacer NaN warning (#14785)
* Fix spacer NaN warning

* Set the min height of spacer block

* Reset the customHeight if it is invalid

* Add local state in spacer

* Update input field's value when using drag handle

aduth added a commit that referenced this pull request Apr 16, 2019

Fix spacer NaN warning (#14785)
* Fix spacer NaN warning

* Set the min height of spacer block

* Reset the customHeight if it is invalid

* Add local state in spacer

* Update input field's value when using drag handle

aduth added a commit that referenced this pull request Apr 16, 2019

Fix spacer NaN warning (#14785)
* Fix spacer NaN warning

* Set the min height of spacer block

* Reset the customHeight if it is invalid

* Add local state in spacer

* Update input field's value when using drag handle

aduth added a commit that referenced this pull request Apr 16, 2019

Fix spacer NaN warning (#14785)
* Fix spacer NaN warning

* Set the min height of spacer block

* Reset the customHeight if it is invalid

* Add local state in spacer

* Update input field's value when using drag handle

This was referenced Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.