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

feat: add support for arrow key navigation #182

Merged
merged 4 commits into from
Oct 26, 2023

Conversation

JMicheli
Copy link
Collaborator

This pull request is a response to issue #123, which requested the left/right arrow keys be usable for previous/next page navigation when reading books.

I implemented this with a new effect in BookReaderScene.tsx which adds a keydown listener and navigates on ArrowLeft and ArrowLeft if the current page isn't the first/last page.

Happy to make any changes needed, I'm less familiar with React so I may not have done this the ideal way.

@aaronleopold
Copy link
Collaborator

aaronleopold commented Oct 24, 2023

Hello 👋 Thanks for your contribution!

I think for #182 your changes will have to be moved to/adjusted for the EpubJsReader. All image-based readers actually already have keyboard interactions for navigation. Since the few different media types have to be consumed in very different ways (largely a different between EPUB vs image-based media), the BookReaderScene is more of a weigh station to route to the proper route and/or reader component. It probably doesn't help that there are 3 routes for book readers in Stump as a result 😅

I sort of (strongly) dislike working with epub.js, so wanted to look into this a little to help. A quick glance at some examples on their repository suggests you would need to use the rendition object for this, maybe somewhere here. Reach out if you need any help moving forward and I'm more than happy to help!

(Also if you're wanting to participate in Hacktoberfest let me know and I'll apply the appropriate label to this PR)

@JMicheli
Copy link
Collaborator Author

Thanks for the info, I'll look into how to make these changes to EpubJSReader and update accordingly.

@JMicheli
Copy link
Collaborator Author

Okay this is what I've got so far. This allows arrow navigation of epubs but it has a problem I haven't yet been able to solve: when the epub is navigated to a new "location" (e.g. going from the main text to the cover), the epub renderer loses focus and the arrow navigation won't work until the user clicks on the screen to focus on it.

I tried to traverse this by adding an additional listener to the window, since that will fire on keydowns when the epub is not in focus, but this doesn't work. Although the event does fire, the rendition_.next() call does nothing. Not totally sure how things are working under the hood yet, but I'll continue investigating. Any ideas in the meantime?

@aaronleopold
Copy link
Collaborator

aaronleopold commented Oct 26, 2023

Honestly I am not entirely sure. I did try out your changes locally, though:

I tried to traverse this by adding an additional listener to the window, since that will fire on keydowns when the epub is not in focus, but this doesn't work.

I've tried a few different browsers and this additional work around you added seems to resolve the issue for me in all of them, strangely. And then removing your line:

window.addEventListener('keydown', keydown_callback)

presents the issue I believe you are describing, where it loses focus after a seemingly random number of page changes. You're welcome to continue debugging, but I'm satisfied with the functionality I am seeing 😄 In the future, once epub chapter streaming is properly implemented, I have a dream of removing epub.js entirely anyways

@JMicheli
Copy link
Collaborator Author

Hmm maybe I just forgot to hit F5 when testing it 😅, because I just rebuilt and it works fine for me as well. I'm going to push a cleaned up commit without all the fixme comments.

Copy link
Collaborator

@aaronleopold aaronleopold left a comment

Choose a reason for hiding this comment

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

Thank you! (Also note that I approved the CI run, but I know that the Docker workflow will fail. Nothing on your end, CI just needs some tweaking)

@aaronleopold aaronleopold merged commit 1df9bf1 into stumpapp:develop Oct 26, 2023
6 of 7 checks passed
@aaronleopold aaronleopold mentioned this pull request Feb 18, 2024
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