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

use react-kbs for shortcut #1408

Open
lpatiny opened this issue Mar 4, 2022 · 11 comments · May be fixed by #1866
Open

use react-kbs for shortcut #1408

lpatiny opened this issue Mar 4, 2022 · 11 comments · May be fixed by #1866
Labels
enhancement New feature or request

Comments

@lpatiny
Copy link
Member

lpatiny commented Mar 4, 2022

Please check how the following library could be used to manage the shortcuts:

https://github.com/zakodium/react-kbs

We should not forget that many NMRium components may be present in the same page.

@lpatiny lpatiny added the enhancement New feature or request label Mar 4, 2022
@hamed-musallam
Copy link
Member

hamed-musallam commented Mar 4, 2022

@lpatiny
I tried to implement but I face a problem when pressing shift + number for example ( shift + 1 ) it will not work and if we take the Alice, for example ( ! ) this will lead to another problem because the keyboard layout will be different between mac and windows and also we have to consider keyboard language.

@hamed-musallam
Copy link
Member

@lpatiny
where going to use in a global context and not local to the specific component because in any way we will lose the focus on the component once edit and click the button .... etc,

that means we will keep the same way of handling the problem by setting a flag in the global state to whether the mouse is over the Displayer or not and check this when we attend to do action.

so I think after I took a look at our code, maybe this will make the code nicer but will not change a lot we will still follow the old way and we can make a meeting for further discussion

@targos
Copy link
Member

targos commented May 11, 2022

react-kbs v2 works better when the focus is on a div, like in nmrium

@lpatiny
Copy link
Member Author

lpatiny commented Oct 28, 2022

@hamed-musallam Could you make a PR that shows the error ? For example the the shortcuts '1', '2', ...

We would need to test in on swiss and franch keyboards.

@hamed-musallam
Copy link
Member

@lpatiny

the demo should be at the side of react-kbs project

@lpatiny
Copy link
Member Author

lpatiny commented Oct 28, 2022

react-kbs is used in some other big react projects and we are wondering was is the problem in your case. So the PR should be in this project.

@hamed-musallam
Copy link
Member

this is the problem that i have #1408 (comment)

@hamed-musallam
Copy link
Member

hamed-musallam commented Oct 28, 2022

the other projects that you mention seem not to have the same use case

for example

1- press "2" to save the current preferences and if you press again it reapplies the preferences that you saved
2- press on shift + 2 to reset the preferences (what I get is @ on the English keyboard layout and " in the German keyboard layout) and the same for most of the numbers which will not work if the keyboard layout is different

@hamed-musallam
Copy link
Member

@lpatiny

we can add the demo here https://zakodium-oss.github.io/react-kbs/

@hamed-musallam
Copy link
Member

@lpatiny I tried to implement but I face a problem when pressing shift + number for example ( shift + 1 ) it will not work and if we take the Alice, for example ( ! ) this will lead to another problem because the keyboard layout will be different between mac and windows and also we have to consider keyboard language.

I was wrong in this example, because ! is the same for German and English keyboard layouts but shift+ 2 is not, and the same for the others shift + 3,4 .... etc

@targos
Copy link
Member

targos commented Oct 29, 2022

https://github.com/zakodium-oss/react-kbs/releases/tag/v2.1.0 adds support for defining shortcuts with code instead of key.

@hamed-musallam hamed-musallam linked a pull request Nov 4, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging a pull request may close this issue.

3 participants