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: Improve initial hover interaction with Tooltip #20219

Merged
merged 8 commits into from
Feb 20, 2020
Merged
88 changes: 3 additions & 85 deletions packages/components/src/range-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,7 @@ import { clamp, noop } from 'lodash';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import {
useCallback,
useRef,
useEffect,
useState,
forwardRef,
} from '@wordpress/element';
import { useRef, useState, forwardRef } from '@wordpress/element';
import { compose, withInstanceId } from '@wordpress/compose';

/**
Expand All @@ -25,6 +19,8 @@ import Button from '../button';
import Dashicon from '../dashicon';

import { color } from '../utils/colors';

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

(We shouldn't want or need the extra lines amongst these imports, unless there's rhyme or reason to it)

import { useControlledRangeValue, useDebouncedHoverInteraction } from './utils';
import RangeRail from './rail';
import SimpleTooltip from './tooltip';
import {
Expand Down Expand Up @@ -262,84 +258,6 @@ const BaseRangeControl = forwardRef(
}
);

/**
* A float supported clamp function for a specific value.
*
* @param {number} value The value to clamp
* @param {number} min The minimum value
* @param {number} max The maxinum value
* @return {number} A (float) number
*/
function floatClamp( value, min, max ) {
return parseFloat( clamp( value, min, max ) );
}

/**
* Hook to store a clamped value, derived from props.
*/
function useControlledRangeValue( { min, max, value: valueProp = 0 } ) {
const [ value, _setValue ] = useState( floatClamp( valueProp, min, max ) );
const valueRef = useRef( value );

const setValue = useCallback(
( nextValue ) => {
_setValue( floatClamp( nextValue, min, max ) );
},
[ _setValue, min, max ]
);

useEffect( () => {
if ( valueRef.current !== valueProp ) {
setValue( valueProp );
valueRef.current = valueProp;
}
}, [ valueRef, valueProp, setValue ] );

return [ value, setValue ];
}

/**
* Hook to encapsulate the debouncing "hover" to better handle the showing
* and hiding of the Tooltip.
*/
function useDebouncedHoverInteraction( {
onShow = noop,
onHide = noop,
onMouseEnter = noop,
onMouseLeave = noop,
timeout = 250,
} ) {
const [ show, setShow ] = useState( false );
const timeoutRef = useRef();

const handleOnMouseEnter = useCallback( ( event ) => {
onMouseEnter( event );

if ( timeoutRef.current ) {
window.clearTimeout( timeoutRef.current );
}

if ( ! show ) {
setShow( true );
onShow();
}
}, [] );

const handleOnMouseLeave = useCallback( ( event ) => {
onMouseLeave( event );

timeoutRef.current = setTimeout( () => {
setShow( false );
onHide();
}, timeout );
}, [] );

return {
onMouseEnter: handleOnMouseEnter,
onMouseLeave: handleOnMouseLeave,
};
}

export const RangeControlNext = compose( withInstanceId )( BaseRangeControl );

export default RangeControlNext;
11 changes: 11 additions & 0 deletions packages/components/src/range-control/stories/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,17 @@ export const customMarks = () => {
);
};

export const multiple = () => {
return (
<Wrapper>
<RangeControlWithState />
<RangeControlWithState />
<RangeControlWithState />
<RangeControlWithState />
</Wrapper>
);
};

const Wrapper = styled.div`
padding: 60px 40px;
`;
96 changes: 96 additions & 0 deletions packages/components/src/range-control/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/**
* External dependencies
*/
import { clamp, noop } from 'lodash';

/**
* WordPress dependencies
*/
import { useCallback, useRef, useEffect, useState } from '@wordpress/element';

/**
* A float supported clamp function for a specific value.
*
* @param {number} value The value to clamp
* @param {number} min The minimum value
* @param {number} max The maxinum value
* @return {number} A (float) number
Copy link
Member

Choose a reason for hiding this comment

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

Two things:

  • Each "column" of the docblock should be aligned, specifically the descriptions in this case.
  • @return should be separated by a newline (and is not subject to alignment with @param parts)
 * @param {number} value The value to clamp
 * @param {number} min   The minimum value
 * @param {number} max   The maxinum value
 *
 * @return {number} A (float) number

*/
function floatClamp( value, min, max ) {
return parseFloat( clamp( value, min, max ) );
}

/**
* Hook to store a clamped value, derived from props.
*/
export function useControlledRangeValue( { min, max, value: valueProp = 0 } ) {
const [ value, _setValue ] = useState( floatClamp( valueProp, min, max ) );
const valueRef = useRef( value );

const setValue = useCallback(
( nextValue ) => {
_setValue( floatClamp( nextValue, min, max ) );
},
[ _setValue, min, max ]
);
aduth marked this conversation as resolved.
Show resolved Hide resolved

useEffect( () => {
if ( valueRef.current !== valueProp ) {
setValue( valueProp );
valueRef.current = valueProp;
}
}, [ valueRef, valueProp, setValue ] );
aduth marked this conversation as resolved.
Show resolved Hide resolved

return [ value, setValue ];
}

/**
* Hook to encapsulate the debouncing "hover" to better handle the showing
* and hiding of the Tooltip.
*/
export function useDebouncedHoverInteraction( {
onShow = noop,
onHide = noop,
onMouseEnter = noop,
onMouseLeave = noop,
timeout = 300,
} ) {
const [ show, setShow ] = useState( false );
const timeoutRef = useRef();

const setDebouncedTimeout = useCallback(
( callback ) => {
if ( timeoutRef.current ) {
window.clearTimeout( timeoutRef.current );
}
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but: window.clearTimeout will not complain if you give it undefined. Thus, the condition is not strictly necessary.

Copy link
Author

@ItsJonQ ItsJonQ Feb 14, 2020

Choose a reason for hiding this comment

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

Oh! Thanks for the heads up :). I'll make that update

Edit: I've noticed the clearTimeout guard in other parts of Gutenberg as well. Maybe something to adjust in the future (when we get around to it 😊 )

Copy link
Member

Choose a reason for hiding this comment

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

I could imagine an argument on the basis of: Being explicit makes it clearer that there's an expectation that it might be unassigned. So even if not necessary, one might think it could help maintainability. For me, I think the benefits of simplifying it are slightly more (both in expressiveness, and smaller bundle size), so I'd lean more on the side of avoiding it.


timeoutRef.current = setTimeout( callback, timeout );
aduth marked this conversation as resolved.
Show resolved Hide resolved
},
[ timeout ]
);

const handleOnMouseEnter = useCallback( ( event ) => {
onMouseEnter( event );

setDebouncedTimeout( () => {
if ( ! show ) {
setShow( true );
onShow();
}
} );
}, [] );

const handleOnMouseLeave = useCallback( ( event ) => {
onMouseLeave( event );

setDebouncedTimeout( () => {
setShow( false );
onHide();
} );
}, [] );

return {
onMouseEnter: handleOnMouseEnter,
onMouseLeave: handleOnMouseLeave,
};
}
Loading