Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Plus sign support #755

Merged

Conversation

leoeuclids
Copy link
Contributor

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

  • Use regex for splitting eventType/keyCombo
  • Add support for keyCombos with a plus sign

@SergeAstapov
Copy link
Contributor

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
Copy link
Contributor

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

@leoeuclids
Copy link
Contributor Author

@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
Copy link
Contributor

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 leoeuclids force-pushed the feature-keycombo-plus branch from a99750d to 3fac1a3 Compare September 12, 2023 13:38
@leoeuclids
Copy link
Contributor Author

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

Done

@leoeuclids
Copy link
Contributor Author

leoeuclids commented Oct 31, 2023

@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?

@leoeuclids leoeuclids force-pushed the feature-keycombo-plus branch from 3fac1a3 to 6e9f009 Compare January 2, 2024 14:37
@benedikt
Copy link

benedikt commented Mar 7, 2024

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

@leoeuclids leoeuclids force-pushed the feature-keycombo-plus branch from 6e9f009 to 7629bd5 Compare March 8, 2024 14:00
@NullVoxPopuli NullVoxPopuli merged commit f173e2c into adopted-ember-addons:master Mar 8, 2024
18 checks passed
@github-actions github-actions bot mentioned this pull request Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants