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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[Slider] Narrow the type of value in callbacks #1241

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
053d791
restrict slider type
seloner Jan 8, 2025
41654ae
proptypes change
seloner Jan 8, 2025
1b5a14d
inline casting
seloner Jan 8, 2025
64b61c8
Merge branch 'master' into fix/slider-types
mj12albert Jan 14, 2025
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
52 changes: 37 additions & 15 deletions packages/react/src/slider/root/SliderRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,24 @@ import { useSliderRoot } from './useSliderRoot';
import { SliderRootContext } from './SliderRootContext';
import { useFieldRootContext } from '../../field/root/FieldRootContext';

type SliderRootType = {
<Value extends number | readonly number[]>(
props: SliderRoot.Props<Value> & {
ref?: React.RefObject<HTMLDivElement>;
Copy link
Author

Choose a reason for hiding this comment

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

This was needed to include the ref into types @mj12albert

},
): React.JSX.Element;
propTypes?: any;
};

/**
* Groups all parts of the slider.
* Renders a `<div>` element.
*
* Documentation: [Base UI Slider](https://base-ui.com/react/components/slider)
*/
const SliderRoot = React.forwardRef(function SliderRoot(
props: SliderRoot.Props,
forwardedRef: React.ForwardedRef<HTMLDivElement>,
) {
export const SliderRoot = React.forwardRef(function SliderRoot<
Value extends number | readonly number[],
>(props: SliderRoot.Props<Value>, forwardedRef: React.ForwardedRef<HTMLDivElement>) {
const {
'aria-labelledby': ariaLabelledby,
className,
Expand Down Expand Up @@ -59,8 +67,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
Copy link
Member

@mj12albert mj12albert Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@seloner It should be possible without casting these two props (the legacy useSelect could also be used as a reference)

  1. Add a type parameter to useSliderRoot as well, e.g. https://github.com/mui/material-ui/blob/master/packages/mui-base/src/useSelect/useSelect.types.ts#L22
  2. Export a type (from useSliderRoot) to use in SliderRoot as the generic, e.g.export type SliderValue<Value> = Value extends number ? number : number[] (reference)

Let me know if you need any help ~

Copy link
Member

Choose a reason for hiding this comment

The 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/

Copy link
Author

Choose a reason for hiding this comment

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

I tried yesterday and I was not able to make it work.
Typescript was not correctly infering the type.
I will check again today with your suggestion and update 👍

Copy link
Author

Choose a reason for hiding this comment

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

@mj12albert
Are you sure your approach is working?
I created a minimal sandbox with the recommended approach but types are not working correctly 🤔
https://stackblitz.com/edit/vitejs-vite-k75jr9vi?file=src%2FApp.tsx
Am I missing something?
I have tried your fork also but it does not work 🤔

orientation,
rootRef: forwardedRef,
step,
Expand Down Expand Up @@ -118,7 +127,7 @@ const SliderRoot = React.forwardRef(function SliderRoot(
<CompositeList elementsRef={slider.thumbRefs}>{renderElement()}</CompositeList>
</SliderRootContext.Provider>
);
});
}) as SliderRootType;

export namespace SliderRoot {
export interface State extends FieldRoot.State {
Expand Down Expand Up @@ -157,7 +166,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,
Expand All @@ -166,8 +175,6 @@ export namespace SliderRoot {
| 'min'
| 'minStepsBetweenValues'
| 'name'
| 'onValueChange'
| 'onValueCommitted'
| 'orientation'
| 'largeStep'
| 'step'
Expand All @@ -179,7 +186,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
Expand All @@ -197,12 +204,27 @@ 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, 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, event: Event) => void;
}
}

export { SliderRoot };
Copy link
Author

Choose a reason for hiding this comment

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

@mj12albert
This was needed because ts error
Parameter props of call signature from exported interface has or is using private name

Copy link
Author

Choose a reason for hiding this comment

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

I managed to bypass this with
1b5a14d


SliderRoot.propTypes /* remove-proptypes */ = {
// ┌────────────────────────────── Warning ──────────────────────────────┐
// │ These PropTypes are generated from the TypeScript type definitions. │
Expand Down Expand Up @@ -288,7 +310,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.
Expand Down
Loading