Skip to content

Commit

Permalink
fix(Slider): bug when options are changing
Browse files Browse the repository at this point in the history
  • Loading branch information
matthprost committed Dec 12, 2024
1 parent 65d862c commit 33d84ef
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 33 deletions.
5 changes: 5 additions & 0 deletions .changeset/nice-dots-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@ultraviolet/ui": patch
---

Fix `<Slider />` having issue when options are changing
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export const Options: StoryFn<typeof Slider> = args => {
<Slider
name="slider"
data-testid="slider"
value={[1, 3]}
value={doubleValue}
double
unit="Mb"
options={options}
Expand Down
43 changes: 23 additions & 20 deletions packages/ui/src/components/Slider/components/DoubleSlider.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useTheme } from '@emotion/react'
import styled from '@emotion/styled'
import { useEffect, useId, useMemo, useRef, useState } from 'react'
import { useCallback, useEffect, useId, useMemo, useRef, useState } from 'react'
import { NumberInputV2 } from '../../NumberInputV2'
import { Stack } from '../../Stack'
import { Text } from '../../Text'
Expand Down Expand Up @@ -177,14 +177,17 @@ export const DoubleSlider = ({
return []
}, [options])

const internalOnChangeRef = useRef((localValue: (number | null)[]) => {
const leftSliderValue = localValue[0] === null ? min : localValue[0]
const rightSliderValue = localValue[1] === null ? max : localValue[1]
const newValues = [leftSliderValue, rightSliderValue]
const internalOnChangeRef = useCallback(
(localValue: (number | null)[]) => {
const leftSliderValue = localValue[0] === null ? min : localValue[0]
const rightSliderValue = localValue[1] === null ? max : localValue[1]
const newValues = [leftSliderValue, rightSliderValue]

setSelectedIndexes(newValues)
onChange?.([Math.min(...newValues), Math.max(...newValues)])
})
setSelectedIndexes(newValues)
onChange?.([Math.min(...newValues), Math.max(...newValues)])
},
[max, min, onChange],
)

// Get slider size (for options)
useEffect(() => {
Expand All @@ -199,30 +202,30 @@ export const DoubleSlider = ({
}, [])

const handleMinChange = (newValue: number) => {
internalOnChangeRef.current([newValue, selectedIndexes[1]])
internalOnChangeRef([newValue, selectedIndexes[1]])
}

const handleMaxChange = (newValue: number) => {
internalOnChangeRef.current([selectedIndexes[0], newValue])
internalOnChangeRef([selectedIndexes[0], newValue])
}

const handleChangeInput = (val: number, side?: 'left' | 'right') => {
if (side === 'left') {
const newValue = Math.max(val, min)
if (selectedIndexes[1]) {
internalOnChangeRef.current([
internalOnChangeRef([
Math.min(newValue, selectedIndexes[1]),
Math.max(newValue, selectedIndexes[1]),
])
} else internalOnChangeRef.current([newValue, selectedIndexes[1]])
} else internalOnChangeRef([newValue, selectedIndexes[1]])

Check warning on line 220 in packages/ui/src/components/Slider/components/DoubleSlider.tsx

View check run for this annotation

Codecov / codecov/patch

packages/ui/src/components/Slider/components/DoubleSlider.tsx#L220

Added line #L220 was not covered by tests
} else if (side === 'right') {
const newValue = Math.min(val, max)
if (selectedIndexes[0]) {
internalOnChangeRef.current([
internalOnChangeRef([
Math.min(newValue, selectedIndexes[0]),
Math.max(newValue, selectedIndexes[0]),
])
} else internalOnChangeRef.current([selectedIndexes[0], newValue])
} else internalOnChangeRef([selectedIndexes[0], newValue])

Check warning on line 228 in packages/ui/src/components/Slider/components/DoubleSlider.tsx

View check run for this annotation

Codecov / codecov/patch

packages/ui/src/components/Slider/components/DoubleSlider.tsx#L228

Added line #L228 was not covered by tests
}
}

Expand All @@ -249,9 +252,9 @@ export const DoubleSlider = ({
if (newVal !== null) {
handleChangeInput(newVal, side)
} else if (side === 'left') {
internalOnChangeRef.current([null, selectedIndexes[1]])
internalOnChangeRef([null, selectedIndexes[1]])
} else if (side === 'right') {
internalOnChangeRef.current([selectedIndexes[0], null])
internalOnChangeRef([selectedIndexes[0], null])
}
}}
onBlur={event => {
Expand All @@ -260,15 +263,15 @@ export const DoubleSlider = ({
if (side === 'left') {
const index = activeValue('left')
if (index === 0) {
internalOnChangeRef.current([min, selectedIndexes[1]])
} else internalOnChangeRef.current([selectedIndexes[0], max])
internalOnChangeRef([min, selectedIndexes[1]])
} else internalOnChangeRef([selectedIndexes[0], max])

Check warning on line 267 in packages/ui/src/components/Slider/components/DoubleSlider.tsx

View check run for this annotation

Codecov / codecov/patch

packages/ui/src/components/Slider/components/DoubleSlider.tsx#L266-L267

Added lines #L266 - L267 were not covered by tests
}

if (side === 'right') {
const index = activeValue('right')
if (index === 0) {
internalOnChangeRef.current([min, selectedIndexes[1]])
} else internalOnChangeRef.current([selectedIndexes[0], max])
internalOnChangeRef([min, selectedIndexes[1]])
} else internalOnChangeRef([selectedIndexes[0], max])

Check warning on line 274 in packages/ui/src/components/Slider/components/DoubleSlider.tsx

View check run for this annotation

Codecov / codecov/patch

packages/ui/src/components/Slider/components/DoubleSlider.tsx#L273-L274

Added lines #L273 - L274 were not covered by tests
}
}
}}
Expand Down
27 changes: 15 additions & 12 deletions packages/ui/src/components/Slider/components/SingleSlider.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useTheme } from '@emotion/react'
import styled from '@emotion/styled'
import { useEffect, useId, useMemo, useRef, useState } from 'react'
import { useCallback, useEffect, useId, useMemo, useRef, useState } from 'react'
import { NumberInputV2 } from '../../NumberInputV2'
import { Stack } from '../../Stack'
import { Text } from '../../Text'
Expand Down Expand Up @@ -121,21 +121,24 @@ export const SingleSlider = ({
return []
}, [options])

const internalOnChangeRef = useRef((newValue: number) => {
setSelectedIndex(newValue ?? min)
onChange?.(newValue ?? min)
})
const internalOnChange = useCallback(
(newValue: number) => {
setSelectedIndex(newValue ?? min)
onChange?.(newValue ?? min)
},
[min, onChange],
)

// Make sure that min <= value <= max
useEffect(() => {
if (value < min) {
internalOnChangeRef.current(min)
internalOnChange(min)
} else if (value > max) {
internalOnChangeRef.current(max)
internalOnChange(max)
} else {
setSelectedIndex(() => value ?? min)
}
}, [value, max, min])
}, [value, max, min, internalOnChange])

// Get slider size
useEffect(() => {
Expand Down Expand Up @@ -179,11 +182,11 @@ export const SingleSlider = ({
unit={typeof suffix === 'string' ? suffix : unit}
onChange={newVal => {
if (newVal) {
internalOnChangeRef.current(newVal)
} else internalOnChangeRef.current(0)
internalOnChange(newVal)
} else internalOnChange(0)
}}
onBlur={event => {
if (!event.target.value) internalOnChangeRef.current(min)
if (!event.target.value) internalOnChange(min)
}}
/>
) : (
Expand Down Expand Up @@ -246,7 +249,7 @@ export const SingleSlider = ({
type="range"
value={selectedIndex}
onChange={event => {
internalOnChangeRef.current(Number.parseFloat(event.target.value))
internalOnChange(Number.parseFloat(event.target.value))
}}
min={min}
max={max}
Expand Down

0 comments on commit 33d84ef

Please sign in to comment.