search

adopted-ember-addons / ember-keyboard

An Ember.js addon for the painless support of keyboard events
http://adopted-ember-addons.github.io/ember-keyboard/
Other
177 stars 58 forks source link

Feature: Plus sign support #755

Closed leoeuclids closed 6 months ago

leoeuclids commented 1 year ago

Why?

Ember-keyboard currently does not support key combos that contain a plus sign and modifier keys, e.g. shift++ and ctrl++. Also, the colon usage was being achieved through a split/join, but it could be replaced by regex match.

What it achieves

Adds support for the plus sign in any configuration and uses regex matching to separate the eventType from the keyCombo.

Upgrade path

No changes are needed, since all previously supported features still work the same way.

Caveats

The plus sign support could be achieved with a regex split, but it requires lookbehind, which has support conflict with Ember 3.8 and IE. For that reason, the solution involves splitting by the plus sign and collapsing two subsequent empty strings into a plus sign. Example: 'Ctrl++' > ['Ctrl', '', ''] > ['Ctrl', '+'] Regex with lookbehind: /(?<=[^+])\+|\+(?=[^+])/g

What this PR does

SergeAstapov commented 12 months ago

Also, the colon usage was being achieved through a split/join, but it could be replaced by regex match.

@leoeuclids while that statement is totally true, was there any problem with that approach? Were there some use cases where it failed?

SergeAstapov commented 12 months ago

@leoeuclids could you please fix prettier/lint errors so we could trigger CI?

leoeuclids commented 12 months ago

@leoeuclids while that statement is totally true, was there any problem with that approach? Were there some use cases where it failed?

Not really, no. It's just an improvement in readability, but nothing more. I'm fine with removing it, if you want.

@leoeuclids could you please fix prettier/lint errors so we could trigger CI?

Will do.

SergeAstapov commented 12 months ago

Not really, no. It's just an improvement in readability, but nothing more. I'm fine with removing it, if you want.

if you don't mind, let's keep PR focused and revert that specific : related change then

leoeuclids commented 12 months ago

if you don't mind, let's keep PR focused and revert that specific : related change then

Done

leoeuclids commented 10 months ago

@SergeAstapov Could you help me with the CI errors?

They seem to be happening on every recent PR, so unrelated to this PR changes? If unrelated, what are we missing to merge this?

benedikt commented 6 months ago

Is there anything missing to get this merged? Would really love to add some keyboard shortcuts with + in them 😄