Fix pitch slider arrow key interaction #4547
Conversation
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@AnvitaPrasad Great effort! As per the expected behavior in the issue description, 'When the PitchSlider widget is initialized, pressing the Up or Down arrow key should move only the slider'. Currently, the PR disables the slider upon initialization, which differs from the intended design. Let's check with the maintainers on the best approach. @walterbender @pikurasa, would you be able to review this? Thanks in advance! |
|
Yes. We want to disable canvas scrolling with the arrow keys when the widget is open. The question remains, which of the multiple pitch slides is impacted by the arrow keys? Perhaps the last one selected? (or the first one -- left-most -- if none have been selected). |
|
Thanks for the feedback @Ubayed-Bin-Sufian and @walterbender I disabled arrow key interactions entirely to avoid conflicts when multiple sliders exist. This ensures consistency, but I can adjust it if needed. Would you prefer directing input to the last selected slider (or the left-most if none are selected)? Let me know, and I’ll update the PR! |
|
I think it would be nice to be able to use the arrows on the last selected slider. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@walterbender I've updated the PR to enable arrow key control for the sliders. The implementation now:
Screen.Recording.2025-03-21.at.2.27.07.PM.mov |
|
nice. |
|
Your PR looks good. Just a few linting issues. Some lines with just spaces. Some extra new lines that are not needed. |
|
✅ All Jest tests passed! This PR is ready to merge. |
I believe I have fixed the linting issues now. |
js/activity.js
Outdated
| if ( | ||
| docById("timbreName") !== null && | ||
| docById("timbreName").classList.contains("hasKeyboard") | ||
| ) { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
still some spaces on this line
There was a problem hiding this comment.
and a few more below. L 3080, 3090, 3098
|
|
||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
|
|
There was a problem hiding this comment.
maybe this can be double space instead of triple space?
| // Move down by a semitone | ||
| slider.value = Math.max(currentValue / PitchSlider.SEMITONE, min); | ||
| } | ||
|
|
There was a problem hiding this comment.
same here... no need for so many empty lines between code blocks.
| return false; | ||
| } | ||
| }; | ||
|
|
| if (id == 0) { | ||
| this.activeSlider = id; | ||
| } | ||
|
|
js/widgets/pitchslider.js
Outdated
| this.activeSlider = id; | ||
| } | ||
|
|
||
|
|
| slider.addEventListener("mousedown", () => { | ||
| this.activeSlider = id; | ||
| }); | ||
|
|
|
✅ All Jest tests passed! This PR is ready to merge. |
|
@walterbender Thanks for pointing out the extra blank lines. I think ive removed the unnecessary spaces now. Let me know if anything else needs fixing. |
Fixes #4466
Description
Fixed the issue where the PitchSlider was overlapping with Blocks when using Up and Down arrow keys. The solution completely disables arrow key interactions when the pitch slider widget is open.
Changes
isActiveflag to the PitchSlider class to track when the slider is active__keyPressedmethod in activity.js to check if the pitch slider is open before processing arrow key eventsScreenshot
Screen.Recording.2025-03-17.at.10.45.07.AM.mov