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

RangeControl: Fix initialPosition to accept 0 #18611

Merged
merged 2 commits into from Dec 17, 2019

Conversation

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Nov 19, 2019

Description

Screen Shot 2019-11-19 at 9 13 12 AM

This update fixes the initialPosition prop for RangeControl to properly handle a value of 0. Previously, the fallback value was checking for truthiness, which does not allow for 0 to work.

Being able to set the value to 0 is important with something like a range slider, as you may not have
an initial value, but you would like the slider to start from the very beginning (with an example range of 0-20).

How has this been tested?

Tested in Storybook. Added new story specifically for this use case.

Types of changes

This update changes how RangeControl handles initialPosition of 0, if there are no values defined. This fix will impact all of the places where this scenario exists with RangeControl.

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.

Resolves #18610

@@ -64,9 +64,12 @@ function RangeControl( {
parseFloat( newValue )
);
};

const initialFallbackValue = ! isUndefined( initialPosition ) ?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 19, 2019

Member

We are using isFinite for checking currentInputValue, would it make sense to use isFinite to check initialPosition too?

This comment has been minimized.

Copy link
@ItsJonQ

ItsJonQ Nov 19, 2019

Author Contributor

Ah yes! That works. Thank you!

@@ -37,6 +38,20 @@ export const _default = () => {
);
};

export const WithoutInitialValue = () => {

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Nov 19, 2019

Member

Should we can this InitialValueZero?

This comment has been minimized.

Copy link
@ItsJonQ

ItsJonQ Nov 19, 2019

Author Contributor

Sure thing!

ItsJonQ added 2 commits Nov 19, 2019
This update fixes the `initialPosition` prop for `RangeControl` to properly handle a value of `0`. Previously, the fallback value was checking for truthiness, which does not allow for `0` to work.

Being able to set the value to `0` is important with something like a range slider, as you may not have
an initial value, but you would like the slider to start from the very beginning (with an example range of 0-20).
@ItsJonQ ItsJonQ force-pushed the fix/range-control-intialposition-0 branch from 602fff8 to 8bceeaf Nov 19, 2019
Copy link
Member

jorgefilipecosta left a comment

LGTM 👍

@jorgefilipecosta jorgefilipecosta merged commit 27f86f6 into master Dec 17, 2019
1 of 2 checks passed
1 of 2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Errored
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/range-control-intialposition-0 branch Dec 17, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
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.