Skip to content

WIP: add support for j/k - vi-style navigation #608

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: add support for j/k - vi-style navigation #608

wants to merge 3 commits into from

Conversation

bmusin
Copy link
Contributor

@bmusin bmusin commented Feb 5, 2018

I want to add suport for h/l (vi-like), F9 (or something else)
to open/close left navigation panel.

What tags/ids of DOM elments should I manipulate to open/close side panel?

@bmusin bmusin changed the title IWIP: add support for j/k - vi-style navigation WIP: add support for j/k - vi-style navigation Feb 5, 2018
@sorin-davidoi
Copy link
Contributor

sorin-davidoi commented Feb 5, 2018

What tags/ids of DOM elments should I manipulate to open/close side panel?

You should not manipulate the DOM directly. I would add another keydown event listener in the sidebar component for this functionality and use the showSidebar and hideSidebar functions instead.

@sorin-davidoi
Copy link
Contributor

For the j/k shortcuts, you should look into how to define multiple keys in the aria-keyshortcuts attribute and update it in index.hbs (it appears in four places).

@bmusin
Copy link
Contributor Author

bmusin commented Feb 5, 2018

@sorin-davidoi, sidebar() doesn't add callbacks for handling key events.
Only chapterNavigation().
Shouldn't it go into chapterNavigation()? It seemse logical to me, since it's not related to toggle button.
Also It isn't directly related to two big buttons on page sides.

@sorin-davidoi
Copy link
Contributor

If it toggles the sidebar, I think it belongs to the sidebar component. There is no harm in adding a second event listener there.

@bmusin
Copy link
Contributor Author

bmusin commented Feb 5, 2018

What about j/k and potentially h/l? Can I handle them in chapterNavigation?

@sorin-davidoi
Copy link
Contributor

Yeah, those should be fine there 👍

@bmusin
Copy link
Contributor Author

bmusin commented Feb 5, 2018

@sorin-davidoi, how should I implement going between chapters, e.g. go from 3.3 to 2.0 or 4.0?
Should I manually parse href?


switch (e.key) {
case 'F9':
if (html.classList.contains("sidebar-hidden")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you extract this into a separate function (since it is the same logic as in the click handler)? We could then just call it from the switch and the click handler would be

sidebarToggleButton.addEventListener('click', toggleSidebar);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did.

@bmusin
Copy link
Contributor Author

bmusin commented Feb 5, 2018

@sorin-davidoi, how should I implement going between chapters, e.g. go from 3.3 to 2.0 or 4.0?
Should I manually parse href?

Maybe chapters/subchapters links should have distinguishing tags, so that JS will go by first URL which has necessary tag?

Don't know how to do it, left TODOs, maybe someone else will complete.

@pickfire
Copy link
Contributor

pickfire commented Apr 13, 2018

Maybe we should focus to <main> by default (maybe with tabindex="0"?) so that users can navigate and after navigating to another page with and .

But it's a nice idea to have vi keys navigation. 👍

@sassman
Copy link

sassman commented Dec 20, 2020

The vi navigation bindings for left / right (j/k) are super handy. What about focusing here only on that, since the code is already there are any objections of merging this?

IMO the sidebar navigation could be handled as a follow-up PR.

Any opinions?

break;
case 'l':
e.preventDefault();
// TODO: add code
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO?

Copy link

Choose a reason for hiding this comment

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

seems that the author abandoned this PR, I created #1408 to follow up on this.

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.

4 participants