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

Revise shortcut handling #1283

Closed
pablosichert opened this issue Oct 19, 2017 · 8 comments
Closed

Revise shortcut handling #1283

pablosichert opened this issue Oct 19, 2017 · 8 comments

Comments

@pablosichert
Copy link
Contributor

pablosichert commented Oct 19, 2017

Is this a bug or feature request?

Code restructuring

What is the current behavior?

Shortcuts somehow work and break sometimes

Which are the steps to reproduce?

Use any shortcut in the application

What is the expected or desired behavior?

Shortcuts should work reliably all the time

992d232 fixed an issue by setting some global prop. Although ideally, there should be a straightforward hierarchy of shortcut handlers, without needing to (seemingly randomly) set props and globally overwrite each other.

Note to IT

  • when testing shortcuts like CTRL+1,2,3 etc pls make sure you have more tabs opened because chrome is also binding those shortcuts for "go to first tab", "go to second tab" etc.
pablosichert added a commit that referenced this issue Nov 6, 2017
Only activate shortcuts for the last mounted QuickActionsContextShortcuts component

Issue #1283
Issue #1324
metas-mk added a commit to metasfresh/metasfresh that referenced this issue Nov 7, 2017
Adding aditional Issues to the Release of next week.
metasfresh/metasfresh-webui-frontend-legacy#1283
Revise shortcut handling
metasfresh/metasfresh-webui-frontend-legacy#1324
Process is started twice when using ctrl-u shortcut
metas-ts pushed a commit that referenced this issue Nov 7, 2017
Only activate shortcuts for the last mounted QuickActionsContextShortcuts component

Issue #1283
Issue #1324

(cherry picked from commit f38f26b)
@metas-ts
Copy link
Member

metas-ts commented Nov 7, 2017

cherry-picked to 2017-11

metas-ts pushed a commit that referenced this issue Nov 10, 2017
Only activate shortcuts for the last mounted QuickActionsContextShortcuts component

Issue #1283
Issue #1324

(cherry picked from commit f38f26b)
pablosichert added a commit that referenced this issue Nov 16, 2017
pablosichert added a commit that referenced this issue Nov 16, 2017
pablosichert added a commit that referenced this issue Nov 16, 2017
pablosichert added a commit that referenced this issue Nov 16, 2017
pablosichert added a commit that referenced this issue Nov 16, 2017
pablosichert added a commit that referenced this issue Nov 16, 2017
pablosichert added a commit that referenced this issue Nov 16, 2017
@teosarca
Copy link
Member

quick checked and found several problems. will document them below.

application crashes:
image

image

@teosarca
Copy link
Member

another issue: CTRL+1,2,3... shortcuts are working very wierd, i.e.

  • sometimes they are not working (might be because it depends on which compenent has the focus?)
  • pressing CTRL+1 and then pressing CTRL+1 again will lead you to first browser tab. Same, CTRL+2 will lead you to second tab etc

@pablosichert
Copy link
Contributor Author

@teosarca reason for that second behavior was, that I reset the whole "key combo" when one key was going up: https://github.com/metasfresh/metasfresh-webui-frontend/blob/5c194e4af4b344c912b5aeaf49e107a373a86848/src/components/Shortcuts/ShortcutProvider.js#L74-L77

What happened:

  1. Hit CTRL // Combo is [CTRL]
  2. Hit 1 // Combo is [CTRL, 1] - CTRL+1 fires
  3. Release 1 // Combo is []
  4. Hit 2 // Combo is [2] - No combo for 2

What should have happened:

  1. Hit CTRL // Combo is [CTRL]
  2. Hit 1 // Combo is [CTRL, 1] - CTRL+1 fires
  3. Release 1 // Combo is [CTRL]
  4. Hit 2 // Combo is [CTRL, 2] - CTRL+2 fires

Going to fix that

@pablosichert
Copy link
Contributor Author

There shouldn't be any focus issues any more, since all hotkeys are handled from one place and all listeners are registered on the top level document element

pablosichert added a commit that referenced this issue Nov 20, 2017
pablosichert added a commit that referenced this issue Nov 20, 2017
pablosichert added a commit that referenced this issue Nov 20, 2017
pablosichert added a commit that referenced this issue Nov 20, 2017
pablosichert added a commit that referenced this issue Nov 20, 2017
@teosarca
Copy link
Member

note to IT: all shortcuts where changed from CTRL+ to ALT+.

@teosarca
Copy link
Member

it seems that some shortcuts are duplicated. see below (javascript console).
thanks @wiadev for reporting it.

image

@metas-dh
Copy link
Member

metas-dh commented Dec 5, 2017

Results of IT1
tested in mf15

checked shortcuts from here: http://docs.metasfresh.org/webui_collection/EN/Keyboard_shortcuts_reference.html

  • select all rows: now [alt]+a: OK
  • Letter, [alt]+r: not working, Handler defined for key sequence "ALT+R" is not a function.
  • Clone, [alt]+rw not working, Handler defined for key sequence "ALT+W" is not a function.

=> follow-up: #1410

@metas-dh metas-dh closed this as completed Dec 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants