[NoJira][BpkSlider] - Fix native event handling for the Slider #3645
Conversation
metalix2
added
patch
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
Browser supportIf this is a visual change, make sure you've tested it in multiple browsers. |
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
1 similar comment
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
metalix2
changed the title
| // needed on document as users can drag the thumb while out of the thumb elements mouse area | ||
| document.addEventListener( | ||
| 'click', | ||
| () => interactionEndHandler(value as number), |
There was a problem hiding this comment.
Why do we need to cast the value? Shouldn't interactionEndHandler expect to handle all the diferent types of values that come from the event listener?
There was a problem hiding this comment.
General comment on this area of the code is that the nested nature of the options/functions etc make it a little difficult to read, is there a way to make it easier? I think, if we are able to not have to pass the casting there is a small increase in readability, eg:
document.addEventListener('click', interactionEndHandler, options)
There was a problem hiding this comment.
Why do we need to cast the value? Shouldn't interactionEndHandler expect to handle all the diferent types of values that come from the event listener?
I thought we had originally needed them but the value remains the same once instanciated.
There was a problem hiding this comment.
General comment on this area of the code is that the nested nature of the options/functions etc make it a little difficult to read, is there a way to make it easier? I think, if we are able to not have to pass the casting there is a small increase in readability, eg:
I'll see If I can refactor it a little.
| ['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes( | ||
| e.key, | ||
| ) && interactionEndHandler(value as number); |
There was a problem hiding this comment.
Should this logic not be within the handler itself?
Even if there are different types of handlers with shared common logic:
// common & shared
function interactionValueParsing(){}
function keyupInteractionHandler (e){
if(!['ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown'].includes( e.key)) return;
const value = interactionValueParsing(e)
// stuff
}There was a problem hiding this comment.
Well It would mean checking for key events when it was a click event. This just sort of wraps and takes care of the custom keyboard logic.
There was a problem hiding this comment.
It would mean checking for key events when it was a click event
I don't understand. Why? Don't we attach the handler twice, once for click and once for keyup - these could just be different handlers, no?
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
| // needed on document as users can drag the thumb while out of the thumb elements mouse area | ||
| signal: controller.signal, | ||
| }); | ||
| thumb.addEventListener('touchend', interactionEndHandler, { |
There was a problem hiding this comment.
touchend needed for mobile devices 👍 Tested in storybook
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3645 to see this build running in a browser. |
Description
Resolve native event bug where dragging over many slider steps result in multiple change events.
This change fixes that once a users
mousedownevent is lifted the"click"will then register a finalchangeevent.Works for keyboard as well the
"keyup"event where a user can hold down the arrow key and the change sent at that moment.Tested across Chrome, Firefox and Safari.
OldSlider.ChangeEvents.mov
Slider.ChangeEvent.mov
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.md