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
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 6 additions & 86 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 @@ -93,7 +89,9 @@ const BaseRangeControl = forwardRef(
}
};

const isCurrentlyFocused = inputRef.current?.matches( ':focus' );
const isThumbFocused = ! disabled && isFocused;

const fillValue = ( ( value - min ) / ( max - min ) ) * 100;
const fillValueOffset = `${ clamp( fillValue, 0, 100 ) }%`;

Expand Down Expand Up @@ -219,7 +217,7 @@ const BaseRangeControl = forwardRef(
className="components-range-control__tooltip"
inputRef={ inputRef }
renderTooltipContent={ renderTooltipContent }
show={ showTooltip || showTooltip }
show={ isCurrentlyFocused || showTooltip }
style={ offsetStyle }
value={ value }
/>
Expand Down Expand Up @@ -262,84 +260,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;
`;
97 changes: 97 additions & 0 deletions packages/components/src/range-control/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**
* 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 = ( nextValue ) => {
_setValue( floatClamp( nextValue, min, max ) );
};
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I don't usually care for _ prefixing, typically because there's usually a better way to distinguish whatever it is we're otherwise assuming to be "the same". In this case, one is responsible for directly setting the state value. The other sets the state value, but within particular constraints. Thus, I could imagine some naming like setValue and something like setClampedValue.


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

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 ) => {
window.clearTimeout( timeoutRef.current );

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();
} );
}, [] );

useEffect( () => {
return () => {
window.clearTimeout( timeoutRef.current );
};
} );

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