search

sciencefair-land / sciencefair

The futuristic, fabulous and free desktop app for working with scientific literature :microscope: :book:
https://sciencefair-app.com
MIT License
603 stars 52 forks source link

Esc key #142

Closed CAYdenberg closed 7 years ago

CAYdenberg commented 7 years ago

Are there any other modals/dialogs? Anything else I'm missing?

blahah commented 7 years ago

@CAYdenberg we should have ctrl/cmd+a to select all results #select . However, this is non-trivial:

It might be useful to break the basic registering of shortcuts from choo components out into a module choo-mousetrap, that allows other choo plugins to register keybindings using events (bus.emit('mousetrap:bind'), bus.emit('mousetrap:bind'), etc.), and handles queueing things until the DOM is ready. I did this with kanye, see choo-kanye, but that doesn't work in electron.

blahah commented 7 years ago

@CAYdenberg but having said the above, this looks great and I think we can just go ahead and merge, then do the above as a separate PR if that suits you? Happy either way, and thanks for the contribution!

CAYdenberg commented 7 years ago

Interesting. I'm not sure I agree with the decision to place keybindings in the model as they are user-interface concerns. If the model listens for the appropriate intention (eg. 'datasources:toggle-shown') then it makes for sense IMO to place a set of keybindings that triggers those intentions in a separate file. The model doesn't care whether those intentions are triggered by mouse clicks or keyboard events. What I'm not clear on though is the pitfall that DOMContentLoaded covers (as I admit to never having heard of choo until reading through your code): is this similar to $(document).ready? Is it fired only once - or many times as the actions are fired and the DOM updates?

I think the pattern you'd want in a generalized choo-mousetrap plugin would be to be able to program: GIVEN some state, WHEN a key event X happens, THEN we bus.emit something. Which is similar to Atom's system for user-defined keybindings. We can definitely make that happen in a less verbose way (my personal preference is just to let the pattern emerge first before crafting a generalized solution).

Anyways, you can merge or not as you see fit - if you fill me in on DOMContentLoaded I'll keep going on this branch or a separate one. :smiley:

BTW - awesome work on this project. I love the momentum this has. :rocket:

blahah commented 7 years ago

Good points.

@CAYdenberg let's get this merged :shipit: and then we can discuss the generalised solution for keybinding in #133