Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Menu): correctly handle isFromKeyboard #596

Merged
merged 12 commits into from
Dec 12, 2018
Merged

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Dec 11, 2018

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 from what-input. This means that our handlers will always take an unactual value from whatInput.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 from what-input.


As were suggested, I made a cleanup there:

  • copied library and removed unused functions
  • add useCapture in addEventListener
  • drop what-input dependency
  • removed isFromKeyboard.ts as util
  • typescriptUtils.ts also removed because they were used only by isFromKeyboard.ts

fix

TODO

  • update CHANGELOG

@@ -0,0 +1,427 @@
// Taken from https://github.com/ten1seven/what-input/blob/master/src/scripts/what-input.js
Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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 in whatInput.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.

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. needs author changes Author needs to implement changes before merge and removed 🚀 ready for review labels Dec 11, 2018
@layershifter layershifter added 🚀 ready for review and removed needs author changes Author needs to implement changes before merge labels Dec 11, 2018
@@ -137,7 +137,7 @@ class RadioGroupItem extends AutoControlledComponent<
accessibility: radioGroupItemBehavior as Accessibility,
}

static autoControlledProps = ['checked', isFromKeyboard.propertyName]
static autoControlledProps = ['checked', 'isFromKeyboard']
Copy link
Member

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?

Copy link
Member Author

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.

src/components/RadioGroup/RadioGroupItem.tsx Show resolved Hide resolved
@layershifter layershifter merged commit 206da99 into master Dec 12, 2018
@layershifter layershifter deleted the fix/whatinput-state branch December 12, 2018 15:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"what-input" library doesn't work correctly when pressing key for first time
5 participants