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: keyboard scrolling with j and k #117

Merged
merged 13 commits into from
Feb 18, 2019

Conversation

frontendwizard
Copy link
Contributor

This is my go on implementing this feature. Not much experience with TypeScript so any tips and reviews are welcome. I'll gladly refactor this to make it better. :)
Both the scrolling amount and the wait time for another scrolling are hardcoded. I'm gonna rename it to constants to make it more readable. So far, what do you all think? The functionality is implemented, you just need to select a column and press j and k to scroll it up and down. Multiple press don't really behave well, so that's why I mitigated this by putting a scrolling flag on event.

@brunolemos
Copy link
Member

brunolemos commented Feb 18, 2019

Thanks for your PR, I just tested it and it worked well!
The way you did it, it solves the problem of scrolling vertically with only the keyboard for each column.

To have full keyboard support, instead of scrolling a fixed amount, we'll need to scroll item by item, and be able to take actions for each item (e.g. press S to save/unsave, etc).

So one change to be made is, in addition to tracking which column is selected, we need to track which item is selected. Instead of using scrollToOffset we could use scrollToItem or maybe scrollToIndex.

Please check https://tweetdeck.twitter.com/, we could make the exact same behavior as them. Play with it by pressing H/J/K/L or the LEFT/UP/DOWN/RIGHT arrows. You'll notice a few things:

  1. You can press the keyboard shortcut any time and it will work, no need to select the column with the number first
  2. It highlights which item is selected
  3. It only scrolls if the selected item is not visible in the screen (scrollToIndex or scrollToItem will probably handle this automatically)
  4. When you press H/L or LEFT/RIGHT it changes column and selects the item that makes more sense, that is aligned in the same place at the screen (not sure how to handle this, maybe there's some method to get the item for a scroll offset)
  5. Since it keeps track of the selected item, you can take action on them, e.g. by pressing F to like/unlike a tweet
  6. It allows you to hold the J/K/etc keys and it will keep moving to the next item without a delay

kapture 2019-02-18 at 5 12 14

About the code, there's some performance optimizations to be made.
For example: const [scrollOffsetY, setScrollOffsetY] = React.useState(0)
On every scroll it's calling setScrollOffsetY and triggering a re-render of the whole list component, which makes it slow.

One alternative could be to make useKeyboardScrolling return a method, for example: const { setScrollPosition} = useKeyboardScrolling(. And inside the useKeyboardScrolling hook, instead of using useState, use useRef, also to avoid re-rendering.

Let me know if you are willing to make these changes.
Any question I can help. Thanks!

@frontendwizard
Copy link
Contributor Author

Awesome, thanks for the review. I'll work on these changes :)

@frontendwizard
Copy link
Contributor Author

Ok. The functionality now is closer to what you specified. Now it's time for performance improvements. I'm using useState only to keep track of the current selected item on the column. How would I use useRef on this case to improve performance? Does this make sense? Legit question.
I removed the scroll tracking since now I'm using scrollToIndex and the functionality feels much better now.

@brunolemos brunolemos self-requested a review February 18, 2019 22:18
@brunolemos
Copy link
Member

It's pretty good now, thanks for your contribution! 🎉🎉🎉

I'll merge and play with it a little bit.

@brunolemos brunolemos merged commit b223ff8 into devhubapp:master Feb 18, 2019
@frontendwizard frontendwizard deleted the feature/keyboard-scrolling branch February 18, 2019 23:07
@frontendwizard
Copy link
Contributor Author

frontendwizard commented Feb 18, 2019

🎉
you're welcome 😃

@brunolemos brunolemos added this to the v0.46.0 milestone Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants