Skip to content
This repository has been archived by the owner on Jun 19, 2019. It is now read-only.

Keyboard shortcuts #57

Merged
merged 35 commits into from
Jan 28, 2013
Merged

Keyboard shortcuts #57

merged 35 commits into from
Jan 28, 2013

Conversation

mdietz
Copy link
Collaborator

@mdietz mdietz commented Jan 23, 2013

Support for simple keyboard shortcuts including ?:help, j/k: next/prev msg, shift+n/p: next/prev channel.

@mdietz mdietz mentioned this pull request Jan 23, 2013
@cespare
Copy link
Collaborator

cespare commented Jan 23, 2013

If you're going to introduce mousetrap, is it possible to get rid of jquery.hotkeys? We have a million vendor js deps.

@bkad
Copy link
Owner

bkad commented Jan 24, 2013

remove trailing whitespace from style.styl and help.mustache

messages = $(".chat-messages.current")
difference = (messages[0].scrollHeight - messages.scrollTop()) - messages.outerHeight()
difference <= 1

scrollMessagesUp: (options = animate: true) ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Refactor this mess

@cespare
Copy link
Collaborator

cespare commented Jan 25, 2013

Issues after trying it out:

  • When the help dialog pops up, if it loses focus or if the users hits 'esc', it should close. Don't force the user to click 'x'.
  • The help dialog needs some styling work. I'm happy to take a crack at it over the next couple of days.
  • Markdown cheatsheet should show the rendered versions. It might be good to make a larger info sheet with more complete user info, perhaps divided into tabs or sections. See Info/user guide page #61.
  • The help dialog should show keyboard shortcuts in a more user-friendly manner. For instance: 'shift_n' -> 'shift+n', 'shift_/' -> '?'
  • We really need a shortcut for focusing and defocusing the text area in order for the shortcuts to really be useful. I think enter or space (or both) to focus, esc to defocus.

This feature is really great. Can't wait to get it polished up and merged :)

@cespare
Copy link
Collaborator

cespare commented Jan 25, 2013

Also, in the interest of getting this feature finished, I would be ok with axing the markdown cheat sheet portion of the help dialog for now. Then we can merge this sooner and we'll take care of that feature as part of #61.

@mdietz
Copy link
Collaborator Author

mdietz commented Jan 25, 2013

I'm going to add the defocus keybinding, restyle the keyboard shortcuts part of the help page, and fix the shift_n keyboard shortcut notation in the help page. Very much in favor of axing the markdown help until we can make a polished version.

@cespare
Copy link
Collaborator

cespare commented Jan 25, 2013

The help dialog looks similar to the message preview dialog, but it's different.

help

preview

  • Different border radius (I think?)
  • Hover highlight on the 'x' (for close) looks different
  • Background is grayed out for preview, but not help. (I would be in favor of graying out in both cases, but much less than the preview is currently grayed out. Say, black with opacity 0.2 for both.)
  • Along with graying out the background, the preview message goes away when it is defocused, but disallows any interaction with the background (e.g., scrolling) while it is visible. It'd be good for the help dialog to have this behavior as well.

I can take care of these things when I get a chance.

@cespare
Copy link
Collaborator

cespare commented Jan 25, 2013

I'd like the shift+n/shift+p shortcuts to wrap around (e.g., shift+p on the first channel goes to the last one). Thoughts?

@bkad
Copy link
Owner

bkad commented Jan 25, 2013

I kind of like the fact that you can type with the markdown cheatsheet up, but I can see why you would want to make the more consistent.

@cespare
Copy link
Collaborator

cespare commented Jan 25, 2013

Paraphrasing from Prat discussion:

If you can interact with the background, it introduces too many corner cases. If the window is small, or if the dialog becomes big, it could cover up (for example) the input box, which could be confusing.

Also, it allows for the possibility of opening multiple modals at once. You can do that right now actually -- open the help dialog and then open preview.

Disallowing interaction with the background while the modal has focus really limits the weird corner cases, and I think that 'click elsewhere to make it go away' is expected behavior. If you look at other examples of these kinds of dialogs on the interwebs, you'll see the same thing:

  • gmail (+ google drive, reader, etc)
  • github
  • bitbucket (doesn't close on defocus, but also doesn't allow interaction)
  • jira (same as bitbucket)
  • trello (insanely detailed shortcut popup on that site)
  • twitter
  • this is what barkeep and vimium do

@mdietz
Copy link
Collaborator Author

mdietz commented Jan 25, 2013

I'm with @cespare on this. The modals should take focus and disable interaction with the rest of the page.

@bkad
Copy link
Owner

bkad commented Jan 25, 2013

okay, you've convinced me

@mdietz
Copy link
Collaborator Author

mdietz commented Jan 25, 2013

Lots of styling tweaks added, finally figured out why modals aren't dismissing correctly (and fixed it for our current modal dialogs), and addressed @cespare 's tweaks in #57 (comment).

This feature should be ready to merge.

@cespare
Copy link
Collaborator

cespare commented Jan 26, 2013

Nice, you did the circular channel switching.

I still have a few style tweaks incoming, I'll push in a bit.

I'd also like to fix the following corner case before we merge: right now, when a modal is open, it's still possible to interact with the app via the shortcuts. I'd like to either disable shortcuts completely when inside a modal, or make any keypress dismiss the modal (and not trigger a shortcut).

Trying to make it fit in a bit better with the rest of prat styles.
@cespare
Copy link
Collaborator

cespare commented Jan 26, 2013

OK, I added a styling commit. Let me know what you guys think.

I'm ready for merge on this whenever. I want to experiment with changing our use of bootstrap modals (and fix the bug I mentioned above) but for now, I think I'm ready to see this merged. It should be useful and reasonably polished as is.

@mdietz
Copy link
Collaborator Author

mdietz commented Jan 26, 2013

I like the style tweaks.

bkad added a commit that referenced this pull request Jan 28, 2013
@bkad bkad merged commit fe3e1ec into master Jan 28, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants