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: Range Control: Reset value when setting range control to empty #9253

Merged
merged 1 commit into from Aug 23, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Aug 22, 2018

Currently, when setting the number input to empty, onChange is called with the value equal to 0 instead of the value being reset.
Fixes: #9191

How has this been tested?

I created a paragraph.
I made the font size equal to 20.
I deleted the font size value and checked in code view that the paragraph was not font size setting applied. In master font size 0 was applied.

@jorgefilipecosta jorgefilipecosta added this to the 3.7 milestone Aug 22, 2018

@jorgefilipecosta jorgefilipecosta self-assigned this Aug 22, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Aug 23, 2018

@tofumatt

This worked well for me in testing. It seems like a good use of an e2e test, if you think you could write one (should be straightforward) that would be great, ping me if you want another review on the test!

const resetValue = () => onChange();
const onChangeValue = ( event ) => {
const newValue = event.target.value;
if ( newValue === '' ) {

This comment has been minimized.

@gziolo

gziolo Aug 23, 2018

Member

Shouldn't it apply Number to cast it to int first and then check if it is greater than 0?

@gziolo

gziolo Aug 23, 2018

Member

Shouldn't it apply Number to cast it to int first and then check if it is greater than 0?

This comment has been minimized.

@gziolo

gziolo Aug 23, 2018

Member

I bet in IE11 you can type abc 😢

@gziolo

gziolo Aug 23, 2018

Member

I bet in IE11 you can type abc 😢

This comment has been minimized.

@tofumatt

tofumatt Aug 23, 2018

Member

What if the value can be below 0? Assuming ranges are always above zero causes bugs like #9254.

@tofumatt

tofumatt Aug 23, 2018

Member

What if the value can be below 0? Assuming ranges are always above zero causes bugs like #9254.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Aug 23, 2018

Member

We may have a RangeControl where 0 is an acceptable input, that's why I only apply the reset when an empty input is passed.

@jorgefilipecosta

jorgefilipecosta Aug 23, 2018

Member

We may have a RangeControl where 0 is an acceptable input, that's why I only apply the reset when an empty input is passed.

This comment has been minimized.

@tofumatt

tofumatt Aug 23, 2018

Member

I just tested in IE and you can type abc but it gets reset to empty on this branch, seems fine.

@tofumatt

tofumatt Aug 23, 2018

Member

I just tested in IE and you can type abc but it gets reset to empty on this branch, seems fine.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Aug 23, 2018

Member

It's not only EI11 even chrome only enforces min and max when input numbers using the arrows but allow non-conformant numbers to be typed by the user. Maybe browsers have some reason for this, and it looks like this component should be a wrapper around the browser implementation to contain the slider and the number input. In this case, the bug was our fault because casting an empty string to a number sets a 0 value and we did not check for that.

@jorgefilipecosta

jorgefilipecosta Aug 23, 2018

Member

It's not only EI11 even chrome only enforces min and max when input numbers using the arrows but allow non-conformant numbers to be typed by the user. Maybe browsers have some reason for this, and it looks like this component should be a wrapper around the browser implementation to contain the slider and the number input. In this case, the bug was our fault because casting an empty string to a number sets a 0 value and we did not check for that.

@jorgefilipecosta jorgefilipecosta merged commit 2d338db into master Aug 23, 2018

2 checks passed

codecov/project 50.87% (+0.01%) compared to d5a4aca
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/range-control-value-reset branch Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment