-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Controls] Range slider a11y and performance improvements #159271
Changes from 13 commits
3b1cd48
2c26893
b38dd7a
b62fa6c
d859706
4f7a0f0
02acc60
90fee54
d4e8729
cbce8a0
a982e6f
15f3495
4a3a8ed
21a3c4b
5fcfae2
dd2e4b6
c353f58
297da5a
0ef557d
a0c1024
ae4638d
6ac785a
d077aae
4f45393
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ export const RANGE_SLIDER_CONTROL = 'rangeSliderControl'; | |
export type RangeValue = [string, string]; | ||
|
||
export interface RangeSliderEmbeddableInput extends DataControlInput { | ||
value: RangeValue; | ||
value?: RangeValue; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This fixes an inconsistency where, even though this was marked as a required part of |
||
} | ||
|
||
export type RangeSliderInputWithType = Partial<RangeSliderEmbeddableInput> & { type: string }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License | ||
* 2.0 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import React from 'react'; | ||
|
||
import { EuiFieldNumber, EuiFormControlLayoutDelimited } from '@elastic/eui'; | ||
|
||
import './range_slider.scss'; | ||
import { RangeValue } from '../../../common/range_slider/types'; | ||
import { useRangeSlider } from '../embeddable/range_slider_embeddable'; | ||
|
||
export const RangeSliderButton = ({ | ||
value, | ||
onClick, | ||
onChange, | ||
}: { | ||
value: RangeValue; | ||
onChange: (newRange: RangeValue) => void; | ||
onClick: (event: React.MouseEvent<HTMLDivElement, MouseEvent>) => void; | ||
}) => { | ||
const rangeSlider = useRangeSlider(); | ||
|
||
const min = rangeSlider.select((state) => state.componentState.min); | ||
const max = rangeSlider.select((state) => state.componentState.max); | ||
const isInvalid = rangeSlider.select((state) => state.componentState.isInvalid); | ||
|
||
const id = rangeSlider.select((state) => state.explicitInput.id); | ||
|
||
const isLoading = rangeSlider.select((state) => state.output.loading); | ||
|
||
return ( | ||
<EuiFormControlLayoutDelimited | ||
fullWidth | ||
onClick={onClick} | ||
isLoading={isLoading} | ||
className="rangeSliderAnchor__button" | ||
data-test-subj={`range-slider-control-${id}`} | ||
startControl={ | ||
<EuiFieldNumber | ||
controlOnly | ||
fullWidth | ||
value={value[0] === String(min) ? '' : value[0]} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
onChange={(event) => { | ||
onChange([event.target.value, value[1]]); | ||
}} | ||
placeholder={String(min)} | ||
isInvalid={isInvalid} | ||
className={'rangeSliderAnchor__fieldNumber'} | ||
data-test-subj={'rangeSlider__lowerBoundFieldNumber'} | ||
/> | ||
} | ||
endControl={ | ||
<EuiFieldNumber | ||
controlOnly | ||
fullWidth | ||
value={value[1] === String(max) ? '' : value[1]} | ||
onChange={(event) => { | ||
onChange([value[0], event.target.value]); | ||
}} | ||
placeholder={String(max)} | ||
isInvalid={isInvalid} | ||
className={'rangeSliderAnchor__fieldNumber'} | ||
data-test-subj={'rangeSlider__upperBoundFieldNumber'} | ||
/> | ||
} | ||
/> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,117 +6,57 @@ | |
* Side Public License, v 1. | ||
*/ | ||
|
||
import React, { FC, useState, useRef } from 'react'; | ||
import { debounce } from 'lodash'; | ||
import React, { FC, useState, useRef, useMemo, useEffect } from 'react'; | ||
|
||
import { | ||
EuiFieldNumber, | ||
EuiText, | ||
EuiInputPopover, | ||
EuiLoadingSpinner, | ||
EuiFlexGroup, | ||
EuiFlexItem, | ||
} from '@elastic/eui'; | ||
import { EuiInputPopover } from '@elastic/eui'; | ||
|
||
import { useRangeSlider } from '../embeddable/range_slider_embeddable'; | ||
import { RangeSliderPopover, EuiDualRangeRef } from './range_slider_popover'; | ||
|
||
import './range_slider.scss'; | ||
import { ControlError } from '../../control_group/component/control_error_component'; | ||
|
||
const INVALID_CLASS = 'rangeSliderAnchor__fieldNumber--invalid'; | ||
import { RangeValue } from '../../../common/range_slider/types'; | ||
import { RangeSliderButton } from './range_slider_button'; | ||
import './range_slider.scss'; | ||
|
||
export const RangeSliderControl: FC = () => { | ||
const rangeRef = useRef<EuiDualRangeRef>(null); | ||
const [isPopoverOpen, setIsPopoverOpen] = useState<boolean>(false); | ||
|
||
const rangeSlider = useRangeSlider(); | ||
|
||
const min = rangeSlider.select((state) => state.componentState.min); | ||
const max = rangeSlider.select((state) => state.componentState.max); | ||
const error = rangeSlider.select((state) => state.componentState.error); | ||
const isInvalid = rangeSlider.select((state) => state.componentState.isInvalid); | ||
|
||
const id = rangeSlider.select((state) => state.explicitInput.id); | ||
const value = rangeSlider.select((state) => state.explicitInput.value) ?? ['', '']; | ||
const isLoading = rangeSlider.select((state) => state.output.loading); | ||
|
||
const hasAvailableRange = min !== '' && max !== ''; | ||
const value = rangeSlider.select((state) => state.explicitInput.value); | ||
const [displayedValue, setDisplayedValue] = useState<RangeValue>(value ?? ['', '']); | ||
|
||
const hasLowerBoundSelection = value[0] !== ''; | ||
const hasUpperBoundSelection = value[1] !== ''; | ||
const debouncedOnChange = useMemo( | ||
() => | ||
debounce((newRange: RangeValue) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice usage of the lodash debounce to keep some of this in the react component rather than putting it into the embeddable class. Also great to see the debounce added! |
||
rangeSlider.dispatch.setSelectedRange(newRange); | ||
}, 750), | ||
[rangeSlider.dispatch] | ||
); | ||
|
||
const lowerBoundValue = parseFloat(value[0]); | ||
const upperBoundValue = parseFloat(value[1]); | ||
const minValue = parseFloat(min); | ||
const maxValue = parseFloat(max); | ||
useEffect(() => { | ||
debouncedOnChange(displayedValue); | ||
}, [debouncedOnChange, displayedValue]); | ||
|
||
// EuiDualRange can only handle integers as min/max | ||
const roundedMin = hasAvailableRange ? Math.floor(minValue) : minValue; | ||
const roundedMax = hasAvailableRange ? Math.ceil(maxValue) : maxValue; | ||
useEffect(() => { | ||
setDisplayedValue(value ?? ['', '']); | ||
}, [value]); | ||
|
||
const button = ( | ||
<button | ||
onClick={() => setIsPopoverOpen(!isPopoverOpen)} | ||
className="rangeSliderAnchor__button" | ||
data-test-subj={`range-slider-control-${id}`} | ||
> | ||
<EuiFlexGroup gutterSize="none" responsive={false}> | ||
<EuiFlexItem> | ||
<EuiFieldNumber | ||
controlOnly | ||
fullWidth | ||
className={`rangeSliderAnchor__fieldNumber ${ | ||
hasLowerBoundSelection && isInvalid ? INVALID_CLASS : '' | ||
}`} | ||
value={hasLowerBoundSelection ? lowerBoundValue : ''} | ||
onChange={(event) => { | ||
rangeSlider.dispatch.setSelectedRange([ | ||
event.target.value, | ||
isNaN(upperBoundValue) ? '' : String(upperBoundValue), | ||
]); | ||
}} | ||
disabled={isLoading} | ||
placeholder={`${hasAvailableRange ? roundedMin : ''}`} | ||
isInvalid={isInvalid} | ||
data-test-subj="rangeSlider__lowerBoundFieldNumber" | ||
/> | ||
</EuiFlexItem> | ||
<EuiFlexItem grow={false}> | ||
<EuiText className="rangeSliderAnchor__delimiter" size="s" color="subdued"> | ||
→ | ||
</EuiText> | ||
</EuiFlexItem> | ||
<EuiFlexItem> | ||
<EuiFieldNumber | ||
controlOnly | ||
fullWidth | ||
className={`rangeSliderAnchor__fieldNumber ${ | ||
hasUpperBoundSelection && isInvalid ? INVALID_CLASS : '' | ||
}`} | ||
value={hasUpperBoundSelection ? upperBoundValue : ''} | ||
onChange={(event) => { | ||
rangeSlider.dispatch.setSelectedRange([ | ||
isNaN(lowerBoundValue) ? '' : String(lowerBoundValue), | ||
event.target.value, | ||
]); | ||
}} | ||
disabled={isLoading} | ||
placeholder={`${hasAvailableRange ? roundedMax : ''}`} | ||
isInvalid={isInvalid} | ||
data-test-subj="rangeSlider__upperBoundFieldNumber" | ||
/> | ||
</EuiFlexItem> | ||
{isLoading ? ( | ||
<EuiFlexItem | ||
grow={false} | ||
className="rangeSliderAnchor__spinner" | ||
data-test-subj="range-slider-loading-spinner" | ||
> | ||
<EuiLoadingSpinner /> | ||
</EuiFlexItem> | ||
) : null} | ||
</EuiFlexGroup> | ||
</button> | ||
<RangeSliderButton | ||
value={displayedValue} | ||
onChange={setDisplayedValue} | ||
onClick={(event) => { | ||
// the popover should remain open if the click target is one of the number inputs | ||
if (isPopoverOpen && event.target instanceof HTMLInputElement) { | ||
return; | ||
} | ||
setIsPopoverOpen(!isPopoverOpen); | ||
}} | ||
/> | ||
); | ||
|
||
return error ? ( | ||
|
@@ -130,15 +70,17 @@ export const RangeSliderControl: FC = () => { | |
className="rangeSlider__popoverOverride" | ||
anchorClassName="rangeSlider__anchorOverride" | ||
panelClassName="rangeSlider__panelOverride" | ||
closePopover={() => setIsPopoverOpen(false)} | ||
closePopover={() => { | ||
setIsPopoverOpen(false); | ||
}} | ||
anchorPosition="downCenter" | ||
attachToAnchor={false} | ||
disableFocusTrap | ||
onPanelResize={(width) => { | ||
rangeRef.current?.onResize(width); | ||
}} | ||
> | ||
<RangeSliderPopover rangeRef={rangeRef} /> | ||
<RangeSliderPopover rangeRef={rangeRef} value={displayedValue} onChange={setDisplayedValue} /> | ||
</EuiInputPopover> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a bug where resetting the selections back to "undefined" / empty would cause unsaved changes:
Before:
Screen.Recording.2023-06-09.at.9.34.40.AM.mov
After:
Screen.Recording.2023-06-09.at.9.36.01.AM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see these stability improvements!