-
Notifications
You must be signed in to change notification settings - Fork 212
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
Addition of explicit value of sliders #211
base: main
Are you sure you want to change the base?
Conversation
Adds explicit value view of the current slider value. The value updates on the fly when the slider is moving.
Fixes issue where the change in the value being displayed causes the slider to be resized on the left.
Fixes possible errors with undefined values and types
return ( | ||
<Container> | ||
<ValueDisplay>{sliderValue}</ValueDisplay> | ||
<SliderInput | ||
ref={inputRef} | ||
{...props} | ||
onChange={(e: React.ChangeEvent<HTMLInputElement>) => { | ||
const value = +e.target.value; | ||
setSliderValue(value); | ||
props.onChange?.(value); | ||
}} | ||
/> | ||
</Container> | ||
); | ||
}; |
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.
so u dont need the if else just for showingSliderValue
return ( | |
<Container> | |
<ValueDisplay>{sliderValue}</ValueDisplay> | |
<SliderInput | |
ref={inputRef} | |
{...props} | |
onChange={(e: React.ChangeEvent<HTMLInputElement>) => { | |
const value = +e.target.value; | |
setSliderValue(value); | |
props.onChange?.(value); | |
}} | |
/> | |
</Container> | |
); | |
}; | |
return ( | |
<Container> | |
{ showingSliderValue && | |
<ValueDisplay>{sliderValue}</ValueDisplay> | |
} | |
<SliderInput | |
ref={inputRef} | |
{...props} | |
onChange={(e: React.ChangeEvent<HTMLInputElement>) => { | |
const value = +e.target.value; | |
setSliderValue(value); | |
props.onChange?.(value); | |
}} | |
/> | |
</Container> | |
); | |
}; |
useEffect(() => { | ||
if (inputRef.current) { | ||
setSliderValue(+inputRef.current.value || 0); // Provide a default value | ||
} | ||
}, [props.value]); |
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.
whats this for? it already initializes to zero... , setting the sliderValue each time props.value changes? doesnt the onChange below already do that?
(props: VIACustomControlProps & {_id: string}) => { | ||
const showSliderValue = useSelector(getShowSliderValue); | ||
|
||
return ( | ||
<ControlRow id={props._id}> | ||
<Label>{props.label}</Label> | ||
<Detail> | ||
{'type' in props ? ( | ||
<VIACustomControl | ||
{...props} | ||
value={props.value && Array.from(props.value)} | ||
showSliderValue={showSliderValue} | ||
/> | ||
) : ( | ||
props.content | ||
)} | ||
</Detail> | ||
</ControlRow> | ||
); | ||
}, |
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.
Shouldn't need to edit this file in general?
} | ||
|
||
export const AccentRange: React.FC<AccentRangeProps> = (props) => { | ||
const {showingSliderValue = false} = props; // Default showSliderValue to false if not provided |
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.
should read the settings directly from here?
This commit aims to provide precise visual feedback on the current value at which the slider is at.
This helps in case the slider is used for precise value input that benefits from a linear input method.
The value updates on the fly when the slider is moving and it's solid with the slider element when resizing the webpage view, to avoid it being wrongly rendered.
Here's a preview of the feature: