Skip to content

Conversation

@sahil-shubham
Copy link

@sahil-shubham sahil-shubham commented Aug 16, 2022

About this PR

Tinykeys doesn't have any support for repeated events, which happen when an user holds a shortcut, a simple solution to this is to check event.repeat before doing what you want to do for a shortcut. But doing this is repetitive as there can be multiple tinykeys instances in a big production app.

I have added a allowRepeat in KeyBindingHandlerOptions, this change wouldn't cause any problems to current users as it defaults to true. But can be made false if needed.

How has this been tested?

  • Built and linked package locally

@sahil-shubham
Copy link
Author

sahil-shubham commented Aug 16, 2022

Open to discussing this PR, we have kept allowRepeat as false by default at our organization, Workduck.

Also running npm install led to some changes in the lockfile.

@jamiebuilds
Copy link
Owner

I don't generally like adding options for things that are fairly straight forward in user code.

tinykeys(window, {
	"A": event => {
    if (event.repeat) return
		// ...
  }
})

I have considered a "middleware" API that allows you to share logic like:

tinykeys(window, {
	"A": event => {
    if (event.repeat) return
		// ...
  }
}, {
  middleware: [
		(event, next) => {
			if (!event.repeat) next()
    }
	]
})

Which would be implemented something like:

let index = 0
function next() {
	let fn = middlewares[index++]
	if (fn) {
		fn(event, next)
	} else {
		cb(event)
	}
}
next()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants