-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Menu): correctly handle isFromKeyboard
#596
Conversation
src/lib/whatInput.ts
Outdated
@@ -0,0 +1,427 @@ | |||
// Taken from https://github.com/ten1seven/what-input/blob/master/src/scripts/what-input.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are copying the whole library, I think might be worth to optimize it a bit, and get rid of things that we are not using. For example, I am not sure about data-whatintent
, as we are reading value from data-whatinput
. Also additional props like ignoreKeys
and so on are not used and can be removed.
Basically, what we need it's just to get info if the last input was 'keyboard'. So I suggest to leave everything that is related to it and remove anything else.
touchstart
and touchend
are triggered only for mobile, so we might not need it also as input won't be changed to 'keyboard' on mobile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but this will mean that we want to adopt it. I'm not sure that actually need to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also don't need fireFunctions
and many other stuff, but as I see: we need to fix our bug and just wait for this function in the module.
And may be, a more better solution that matches React's ideas. Because even with setInput()
it will still be a monkey patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can start with removing unused things (because it's easy and fast :)) as waiting for this thing to be fixed in a module takes an unknown time.
Reinventing something new, more in React way, might be considered for some future RFC, but at least we need to get back from authors that they won't fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please understand my position there, we have two options.
Option 1. We want to get an enchantment in the package and we will temporary copy its code as is. We know that this code is stable and working, we should not touch it at all.
Option 2. We fork the package and adopt it for our needs. In this case I of course will remove all unused options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 3: Let's try deleting as much of this as possible. The isFromKeyboard
abstraction currently has lots of code that is unnecessary. I'm also not sure if we need this utility or if it can be done in a more simple way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworked this as you suggested guys.
- drop
what-input
- removed
isFromKeyboard.ts
with util typescriptUtils.ts
also removed- removed unused functions and
intent
inwhatInput.ts
For now, we have only a isFromKeyboard()
function. Issue with events priority was resolved by useCapture
.
However, I would be glad if matching changes were done in third party dependency and will be able to remove whatInput.ts
from our code.
@@ -137,7 +137,7 @@ class RadioGroupItem extends AutoControlledComponent< | |||
accessibility: radioGroupItemBehavior as Accessibility, | |||
} | |||
|
|||
static autoControlledProps = ['checked', isFromKeyboard.propertyName] | |||
static autoControlledProps = ['checked', 'isFromKeyboard'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense for isFromKeyboard
to be autocontrolled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really strange to me, agree. Will remove this.
Fixes #434.
I investigated the issue a bit more and it comes from our side. We have own event listener for
onFocus
and it has the higher priority than a handler fromwhat-input
. This means that our handlers will always take an unactual value fromwhatInput.ask()
.I also tried to play around with a wrapper component (via Custom Callbacks) that will handle this properly, but didn't reach a good and simple result.
isFromKeyboard
is respected only when component is focused, this means that we need separately handle focus and blur events and also events fromwhat-input
.As were suggested, I made a cleanup there:
useCapture
inaddEventListener
what-input
dependencyisFromKeyboard.ts
as utiltypescriptUtils.ts
also removed because they were used only byisFromKeyboard.ts
TODO