Improve Slider Scrolling #8321
Improve Slider Scrolling #8321
Conversation
📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| if (sliderElement) { | ||
| sliderElement.addEventListener("wheel", handleWheelEvent, { passive: false }); | ||
| } | ||
| }, [handleWheelEvent]); |
There was a problem hiding this comment.
I was a little surprised about this solution.
In my view the dependency that makes sense would be sliderRef.current. I read that components are not rerendered if refs change, which makes sense because the concepts behind refs is to reference objects that are not needed for rendering (according to react.dev. I thought that hooks would still be updated, but this must be the wrong assumption.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/javascripts/components/slider.tsx (1)
50-71: Consider simplifying the wheel event handler conditions.The wheel event handler implementation is correct but could be more maintainable.
Consider this refactoring to improve readability:
const handleWheelEvent = useCallback( (event: { preventDefault: () => void; deltaY: number }) => { - // differentiate between single value and range slider - if (onWheelDisabled || value == null || min == null || max == null || !isFocused.current) - return _.noop; + if (onWheelDisabled || !isFocused.current || value == null || min == null || max == null) { + return; + } + + event.preventDefault(); + const diff = getWheelStepFromEvent(ensuredStep, event.deltaY, wheelStep); + if (range === false || range == null) { - event.preventDefault(); - const newValue = value - getWheelStepFromEvent(ensuredStep, event.deltaY, wheelStep); + const newValue = value - diff; const clampedNewValue = clamp(min, newValue, max); if (onChange != null) onChange(clampedNewValue); } else if (range === true || typeof range === "object") { - event.preventDefault(); - const diff = getWheelStepFromEvent(ensuredStep, event.deltaY, wheelStep); const newLowerValue = Math.round(value[0] + diff); const newUpperValue = Math.round(value[1] - diff); const clampedNewLowerValue = clamp(min, newLowerValue, Math.min(newUpperValue, max)); const clampedNewUpperValue = clamp(newLowerValue, newUpperValue, max); if (onChange != null) onChange([clampedNewLowerValue, clampedNewUpperValue]); } }, [value, min, max, onChange, range, onWheelDisabled], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/components/slider.tsx(4 hunks)frontend/javascripts/oxalis/view/components/setting_input_views.tsx(2 hunks)
🔇 Additional comments (4)
frontend/javascripts/components/slider.tsx (3)
5-5: LGTM! Clean prop and import additions.The new optional callback prop and React hooks imports are appropriate for implementing the reset and focus-based scrolling functionality.
Also applies to: 13-13
47-48: LGTM! Proper focus management implementation.Using useRef for focus tracking is the correct approach as it persists across renders without causing re-renders.
87-106: LGTM! Well-implemented reset functionality.The double-click handler correctly implements the reset-to-default functionality with proper type handling. The focus/blur handlers and touchAction style are appropriate additions.
frontend/javascripts/oxalis/view/components/setting_input_views.tsx (1)
197-200: LGTM! Clean implementation of reset functionality.The resetToDefaultValue method is properly implemented with null checks, and the onResetToDefault prop is correctly connected to the Slider component.
Also applies to: 218-218
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
16-16: Consider more concise wording.The phrase "In order to" can be shortened for better readability.
-- Improved the scrolling behaviour of sliders: In order to scroll them, sliders need to be focused. It is also prevented that the parent element is scrolled together with the slider itself. [#8321](https://github.com/scalableminds/webknossos/pull/8321) ++ Improved the scrolling behaviour of sliders: Sliders must be focused to scroll them. Additionally, parent element scrolling is prevented when using the slider. [#8321](https://github.com/scalableminds/webknossos/pull/8321)🧰 Tools
🪛 LanguageTool
[style] ~16-~16: Consider a shorter alternative to avoid wordiness.
Context: ...ved the scrolling behaviour of sliders: In order to scroll them, sliders need to be focused...(IN_ORDER_TO_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/components/slider.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/components/slider.tsx
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[style] ~16-~16: Consider a shorter alternative to avoid wordiness.
Context: ...ved the scrolling behaviour of sliders: In order to scroll them, sliders need to be focused...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
CHANGELOG.unreleased.md (1)
16-16: LGTM! The changelog entry is well-structured.The entry correctly documents the slider improvements, follows the changelog format, and includes the appropriate PR reference.
🧰 Tools
🪛 LanguageTool
[style] ~16-~16: Consider a shorter alternative to avoid wordiness.
Context: ...ved the scrolling behaviour of sliders: In order to scroll them, sliders need to be focused...(IN_ORDER_TO_PREMIUM)
There was a problem hiding this comment.
cool! the code looks good and testing went smoothly mostly, too. I noticed two things, though:
-
double clicking the opacity slider for a segmentation layer, resets the value to 100. however, the default value is 20 for segmentation layers. can you have a look whether it's easy to fix this?
-
I already noticed this on master and maybe it's unrelated to the new slider wrapper, but could you please doublecheck this: when dragging the slider and moving on the y axis, text in the UI is selected. this is not critical, but maybe related to the following problem: sometimes the cursor changes into a "grab" pointer and the slider knob does not move anymore. if I release the mouse button, the knob is still moved when I move the mouse. i have to click again to get rid of it. can you try to reproduce this and afterwards temporarily remove the custom slider component and test whether the problem still exists? I can provoke this by clicking and holding for a moment and only then start to move my mouse.
https://github.com/user-attachments/assets/036f8861-6083-4a46-863e-c9981903e7d9
https://github.com/user-attachments/assets/4da1f440-1b60-4da7-9edd-b7ae339fddf1
by the way, the one moment where the slider jumps to 255, I did a doubleclick, so this is fine :)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/javascripts/components/slider.tsx (2)
26-31: Simplify the wheel step calculation.While the edge case handling is good, the function can be simplified by combining conditions.
const getWheelStepFromEvent = (step: number, deltaY: number, wheelStep: number) => { - const absDeltaY = Math.abs(deltaY); - if (absDeltaY === 0 || step === 0) return 0; + if (!deltaY || !step) return 0; // Make sure that result is a multiple of step return step * Math.round((wheelStep * deltaY) / Math.abs(deltaY) / step); };
50-90: Extract range slider logic for better readability.The wheel event handler is well-implemented with proper cleanup, but the range slider logic could be extracted to improve readability.
+ const handleRangeSliderWheel = (diff: number) => { + const newLowerValue = Math.round(value[0] + diff); + const newUpperValue = Math.round(value[1] - diff); + const clampedNewLowerValue = clamp(min, newLowerValue, Math.min(newUpperValue, max)); + const clampedNewUpperValue = clamp(newLowerValue, newUpperValue, max); + onChange([clampedNewLowerValue, clampedNewUpperValue]); + }; + const handleWheelEvent = useCallback( (event: { preventDefault: () => void; deltaY: number }) => { if ( onWheelDisabled || value == null || min == null || max == null || - !isFocused.current || + !isFocused || onChange == null ) return; event.preventDefault(); const diff = getWheelStepFromEvent(ensuredStep, event.deltaY, wheelStep); if (range === false || range == null) { const newValue = value - diff; const clampedNewValue = clamp(min, newValue, max); onChange(clampedNewValue); } else if (range === true || typeof range === "object") { - const newLowerValue = Math.round(value[0] + diff); - const newUpperValue = Math.round(value[1] - diff); - const clampedNewLowerValue = clamp(min, newLowerValue, Math.min(newUpperValue, max)); - const clampedNewUpperValue = clamp(newLowerValue, newUpperValue, max); - onChange([clampedNewLowerValue, clampedNewUpperValue]); + handleRangeSliderWheel(diff); } }, - [value, min, max, onChange, range, onWheelDisabled], + [value, min, max, onChange, range, onWheelDisabled, isFocused], );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/components/slider.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (1)
frontend/javascripts/components/slider.tsx (1)
10-14: LGTM: Well-typed prop addition.The new
onResetToDefaultprop is properly typed as an optional callback, maintaining backward compatibility.
@philippotto did I understand it correctly that this is happening in the current master, too? I'm asking because I can only reproduce it on this branch. To be honest I am not 100% sure why this is happening. I think it has something to do with the focusing but I didnt dive deep. I may have found an easy solution though, so maybe thats enough :) edit: I was able to reproduce the second issue on wk.org. I think my solution should fix both, by disabling |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
frontend/javascripts/components/slider.tsx (3)
47-48:⚠️ Potential issueSwitch to useState for focus management.
Using
useReffor focus state could lead to race conditions in concurrent mode.Apply this fix:
- const isFocused = useRef(false); + const [isFocused, setIsFocused] = useState(false); const sliderRef = useRef<HTMLDivElement>(null);
99-109:⚠️ Potential issueFix else branch placement in double-click handler.
The
elsebranch is incorrectly placed outside the if block.Apply this fix:
const handleDoubleClick: React.MouseEventHandler<HTMLDivElement> = (event) => { if ( event.target instanceof HTMLElement && event.target.className.includes("ant-slider-handle") && defaultValue != null - ) - if (onResetToDefault != null) onResetToDefault(); + ) { + if (onResetToDefault != null) { + onResetToDefault(); + } else { // @ts-ignore Argument of type 'number | number[]' is not assignable to parameter of type 'number'. //TypeScript doesn't understand that onChange always takes the type of defaultValue. - else onChange(defaultValue); + onChange(defaultValue); + } + } };
115-117: 🛠️ Refactor suggestionUpdate focus handlers to use setState.
The focus handlers should use setState for consistency with the earlier suggestion.
Apply this fix:
- style={{ flexGrow: 1, touchAction: "none", userSelect: isFocused.current ? "none" : "auto" }} - onFocus={() => (isFocused.current = true)} - onBlur={() => (isFocused.current = false)} + style={{ flexGrow: 1, touchAction: "none", userSelect: isFocused ? "none" : "auto" }} + onFocus={() => setIsFocused(true)} + onBlur={() => setIsFocused(false)}
🧹 Nitpick comments (1)
frontend/javascripts/components/slider.tsx (1)
50-77: Optimize guard clauses in wheel event handler.The implementation is correct but can be simplified.
Combine the conditions and use early return:
const handleWheelEvent = useCallback( (event: { preventDefault: () => void; deltaY: number }) => { - if ( - onWheelDisabled || - value == null || - min == null || - max == null || - !isFocused.current || - onChange == null - ) - return; + if (onWheelDisabled || !isFocused.current) return; + if (value == null || min == null || max == null || onChange == null) return; event.preventDefault();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/javascripts/components/slider.tsx(4 hunks)frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx(3 hunks)frontend/javascripts/types/schemas/dataset_view_configuration_defaults.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (7)
frontend/javascripts/types/schemas/dataset_view_configuration_defaults.ts (2)
33-40: Great refactoring ofgetSpecificDefaultsForLayer!The function signature has been improved by:
- Accepting simpler primitive types instead of the complex
APIDataLayertype- Making the color layer check explicit through a boolean parameter
- Following a more descriptive naming convention
This change reduces coupling and improves maintainability.
76-78: LGTM - Function call correctly updated!The call to
getSpecificDefaultsForLayerhas been properly adapted to use the new signature.frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx (3)
133-133: LGTM - Import statement updated!The import statement correctly reflects the renamed function.
1014-1019: LGTM - Proper usage of the new function!The
getSpecificDefaultsForLayerfunction is called with the correct parameters and its result is appropriately used for the layer settings.
1046-1046: LGTM - Correct usage of defaults!The alpha default value is properly obtained from the layer-specific defaults.
frontend/javascripts/components/slider.tsx (2)
5-5: LGTM! Clean type definition for the reset functionality.The optional
onResetToDefaultprop is well-defined and the imports are properly updated.Also applies to: 13-13
27-28: LGTM! Improved robustness of wheel step calculation.The additional checks for zero values prevent potential edge cases and ensure consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/components/slider.tsx (3)
10-14: Consider improving type safety for onResetToDefault.The
onResetToDefaultcallback is correctly added to the SliderProps type. However, consider making it more type-safe by ensuring it's only available whendefaultValueis provided.type SliderProps = (SliderSingleProps | SliderRangeProps) & { wheelFactor?: number; onWheelDisabled?: boolean; - onResetToDefault?: () => void; + onResetToDefault?: NonNullable<SliderSingleProps['defaultValue']> extends never ? never : () => void; };
50-77: Consider extracting range slider logic.The wheel event handler handles both single and range sliders. Consider extracting the range slider logic into a separate function for better maintainability.
+const handleRangeSliderWheel = ( + min: number, + max: number, + value: [number, number], + diff: number, +) => { + const newLowerValue = Math.round(value[0] + diff); + const newUpperValue = Math.round(value[1] - diff); + const clampedNewLowerValue = clamp(min, newLowerValue, Math.min(newUpperValue, max)); + const clampedNewUpperValue = clamp(newLowerValue, newUpperValue, max); + return [clampedNewLowerValue, clampedNewUpperValue] as [number, number]; +}; const handleWheelEvent = useCallback( (event: { preventDefault: () => void; deltaY: number }) => { // ... early returns ... if (range === false || range == null) { const newValue = value - diff; const clampedNewValue = clamp(min, newValue, max); onChange(clampedNewValue); } else if (range === true || typeof range === "object") { - const newLowerValue = Math.round(value[0] + diff); - const newUpperValue = Math.round(value[1] - diff); - const clampedNewLowerValue = clamp(min, newLowerValue, Math.min(newUpperValue, max)); - const clampedNewUpperValue = clamp(newLowerValue, newUpperValue, max); - onChange([clampedNewLowerValue, clampedNewUpperValue]); + onChange(handleRangeSliderWheel(min, max, value as [number, number], diff)); } }, [value, min, max, onChange, range, onWheelDisabled], );
99-112: Remove @ts-ignore by improving type handling.The TypeScript ignore comment can be avoided by properly typing the onChange handler.
const handleDoubleClick: React.MouseEventHandler<HTMLDivElement> = (event) => { if ( event.target instanceof HTMLElement && event.target.className.includes("ant-slider-handle") && defaultValue != null ) if (onResetToDefault != null) { onResetToDefault(); } else { - // @ts-ignore Argument of type 'number | number[]' is not assignable to parameter of type 'number'. - //TypeScript doesn't understand that onChange always takes the type of defaultValue. - onChange(defaultValue); + onChange(defaultValue as typeof value); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/components/slider.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (1)
frontend/javascripts/components/slider.tsx (1)
Learnt from: knollengewaechs
PR: scalableminds/webknossos#8321
File: frontend/javascripts/components/slider.tsx:0-0
Timestamp: 2025-02-05T20:16:45.712Z
Learning: When tracking focus state that needs to be accessed by native event listeners (e.g., wheel events), use `useRef` instead of `useState` to ensure the event handlers always have access to the current value. This is because event listeners added via `addEventListener` would capture the initial state value if `useState` were used.
🔇 Additional comments (2)
frontend/javascripts/components/slider.tsx (2)
47-48: LGTM! Correct usage of useRef for focus state.Using
useReffor focus state is the right choice here since it needs to be accessed by the wheel event listener. This aligns with the best practice of using refs for values that shouldn't trigger re-renders.
115-121: LGTM! Good handling of user interactions.The implementation correctly:
- Prevents text selection during slider interaction with
userSelect- Disables touch actions to prevent interference with wheel events
- Manages focus state appropriately
|
thank you for the thorough review and for dog-fooding the slider! 😊 |
2 participants
URL of deployed dev instance (used for testing):
Steps to test:
(- you can also test this with double clicking the slider for the brush size in the tool bar (having the brush tool active): it should be reset to 50)
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)