-
-
Notifications
You must be signed in to change notification settings - Fork 95
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
[Slider] Narrow the type of value
in callbacks
#1241
Changes from 9 commits
053d791
41654ae
1b5a14d
64b61c8
291cf4b
7ab9dec
3633039
701351c
af634d7
2908f2d
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 |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import * as React from 'react'; | ||
import { expectType } from '#test-utils'; | ||
import { Slider } from '@base-ui-components/react/slider'; | ||
|
||
const value: number = 25; | ||
const array = [25]; | ||
|
||
const singleValueWithOnValueChange = ( | ||
<Slider.Root value={value} onValueChange={(v) => expectType<number, typeof v>(v)} /> | ||
); | ||
const singleDefaultValueWithOnValueChange = ( | ||
<Slider.Root defaultValue={25} onValueChange={(v) => expectType<number, typeof v>(v)} /> | ||
); | ||
|
||
const arrayValueWithOnValueChange = ( | ||
<Slider.Root value={array} onValueChange={(v) => expectType<number[], typeof v>(v)} /> | ||
); | ||
const arrayDefaultValueWithOnValueChange = ( | ||
<Slider.Root defaultValue={[25]} onValueChange={(v) => expectType<number[], typeof v>(v)} /> | ||
); | ||
|
||
const singleValueWithOnValueCommitted = ( | ||
<Slider.Root value={value} onValueCommitted={(v) => expectType<number, typeof v>(v)} /> | ||
); | ||
const singleDefaultValueWithOnValueCommitted = ( | ||
<Slider.Root defaultValue={25} onValueCommitted={(v) => expectType<number, typeof v>(v)} /> | ||
); | ||
|
||
const arrayValueWithOnValueCommitted = ( | ||
<Slider.Root value={array} onValueCommitted={(v) => expectType<number[], typeof v>(v)} /> | ||
); | ||
const arrayDefaultValueWithOnValueCommitted = ( | ||
<Slider.Root defaultValue={[25]} onValueCommitted={(v) => expectType<number[], typeof v>(v)} /> | ||
); | ||
|
||
const singleValueExplicitTypeAnnotation = ( | ||
<Slider.Root<number> onValueChange={(v) => expectType<number, typeof v>(v)} /> | ||
); | ||
const arrayValueExplicitTypeAnnotation = ( | ||
<Slider.Root<number[]> onValueChange={(v) => expectType<number[], typeof v>(v)} /> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,8 +18,8 @@ import { useFieldRootContext } from '../../field/root/FieldRootContext'; | |
* | ||
* Documentation: [Base UI Slider](https://base-ui.com/react/components/slider) | ||
*/ | ||
const SliderRoot = React.forwardRef(function SliderRoot( | ||
props: SliderRoot.Props, | ||
const SliderRoot = React.forwardRef(function SliderRoot<Value extends number | readonly number[]>( | ||
props: SliderRoot.Props<Value>, | ||
forwardedRef: React.ForwardedRef<HTMLDivElement>, | ||
) { | ||
const { | ||
|
@@ -59,8 +59,9 @@ const SliderRoot = React.forwardRef(function SliderRoot( | |
min, | ||
minStepsBetweenValues, | ||
name: nameProp ?? '', | ||
onValueChange: onValueChangeProp ?? NOOP, | ||
onValueCommitted: onValueCommittedProp ?? NOOP, | ||
onValueChange: (onValueChangeProp as useSliderRoot.Parameters['onValueChange']) ?? NOOP, | ||
onValueCommitted: | ||
(onValueCommittedProp as useSliderRoot.Parameters['onValueCommitted']) ?? NOOP, | ||
Comment on lines
+62
to
+64
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. @seloner It should be possible without casting these two props (the legacy
Let me know if you need any help ~ 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. @seloner See if this helps: https://github.com/mj12albert/base-ui/commits/slider-value-type/ 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. I tried yesterday and I was not able to make it work. 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. @mj12albert 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. @seloner You are right ~ it "works" as in there are no errors but (similar to the now deprecated useSelect) we still need to cast some stuff in the end (16ff230), and still the DX isn't good 😢 With this approach you'd still need to manually specify this: https://github.com/mui/material-ui/blob/master/docs/data/base/components/select/UnstyledSelectMultiple.tsx#L25 RA is similar: https://react-spectrum.adobe.com/react-aria/Slider.html#controlled-value Can't believe I didn't notice before but Radix's slider always takes arrays: https://github.com/radix-ui/website/blob/main/components/demos/Slider/css-modules/index.jsx#L7 So anyway, I'm ok with casting the callbacks like that to make it work, what do you think @michaldudak ? (I'm pretty sure @vladmoroz would absolutely hate anything that requires users to touch generics) |
||
orientation, | ||
rootRef: forwardedRef, | ||
step, | ||
|
@@ -118,7 +119,14 @@ const SliderRoot = React.forwardRef(function SliderRoot( | |
<CompositeList elementsRef={slider.thumbRefs}>{renderElement()}</CompositeList> | ||
</SliderRootContext.Provider> | ||
); | ||
}); | ||
}) as { | ||
<Value extends number | readonly number[]>( | ||
props: SliderRoot.Props<Value> & { | ||
ref?: React.RefObject<HTMLDivElement>; | ||
}, | ||
): React.JSX.Element; | ||
propTypes?: any; | ||
}; | ||
|
||
export namespace SliderRoot { | ||
export interface State extends FieldRoot.State { | ||
|
@@ -157,7 +165,7 @@ export namespace SliderRoot { | |
values: readonly number[]; | ||
} | ||
|
||
export interface Props | ||
export interface Props<Value extends number | readonly number[] = number | readonly number[]> | ||
extends Partial< | ||
Pick< | ||
useSliderRoot.Parameters, | ||
|
@@ -166,8 +174,6 @@ export namespace SliderRoot { | |
| 'min' | ||
| 'minStepsBetweenValues' | ||
| 'name' | ||
| 'onValueChange' | ||
| 'onValueCommitted' | ||
| 'orientation' | ||
| 'largeStep' | ||
| 'step' | ||
|
@@ -179,7 +185,7 @@ export namespace SliderRoot { | |
* | ||
* To render a controlled slider, use the `value` prop instead. | ||
*/ | ||
defaultValue?: number | readonly number[]; | ||
defaultValue?: Value; | ||
/** | ||
* Whether the component should ignore user interaction. | ||
* @default false | ||
|
@@ -197,10 +203,30 @@ export namespace SliderRoot { | |
* The value of the slider. | ||
* For ranged sliders, provide an array with two values. | ||
*/ | ||
value?: number | readonly number[]; | ||
value?: Value; | ||
/** | ||
* Callback function that is fired when the slider's value changed. | ||
* | ||
* @param {number | number[]} value The new value. | ||
* @param {Event} event The corresponding event that initiated the change. | ||
* You can pull out the new value by accessing `event.target.value` (any). | ||
* @param {number} activeThumbIndex Index of the currently moved thumb. | ||
*/ | ||
onValueChange?: ( | ||
value: Value extends number ? number : Value, | ||
event: Event, | ||
activeThumbIndex: number, | ||
) => void; | ||
/** | ||
* Callback function that is fired when the `pointerup` is triggered. | ||
* | ||
* @param {number | number[]} value The new value. | ||
* @param {Event} event The corresponding event that initiated the change. | ||
* **Warning**: This is a generic event not a change event. | ||
*/ | ||
onValueCommitted?: (value: Value extends number ? number : Value, event: Event) => void; | ||
} | ||
} | ||
|
||
export { SliderRoot }; | ||
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. @mj12albert 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. I managed to bypass this with |
||
|
||
SliderRoot.propTypes /* remove-proptypes */ = { | ||
|
@@ -288,7 +314,7 @@ SliderRoot.propTypes /* remove-proptypes */ = { | |
/** | ||
* Callback function that is fired when the slider's value changed. | ||
* | ||
* @param {number | readonly number[]} value The new value. | ||
* @param {number | number[]} value The new value. | ||
mj12albert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param {Event} event The corresponding event that initiated the change. | ||
* You can pull out the new value by accessing `event.target.value` (any). | ||
* @param {number} activeThumbIndex Index of the currently moved thumb. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
export { createRenderer } from './createRenderer'; | ||
export { describeConformance } from './describeConformance'; | ||
export { isJSDOM } from './utils'; | ||
export * from './utils'; |
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.
Testing in vscode locally, I noticed that when using

defaultValue
andonValueChange
the type isn'tnumber
:Using controlled mode (

value
) it's correct:Uncontrolled range slider also seems correct:

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 is actually TypeScript's valid behavior. When you set
defaultValue={25}
, the type ofdefaultValue
is set to a constant - literal25
. It's the same as if you wroteconst v = 25
- v's type is25
.A way around it would be to widen the type in the callbacks: