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

Navigation fixes #108

Merged
merged 2 commits into from
Mar 26, 2023
Merged

Navigation fixes #108

merged 2 commits into from
Mar 26, 2023

Conversation

dancamdev
Copy link
Collaborator

@dancamdev dancamdev commented Mar 25, 2023

Fixes #45

  • buttons now mimic native browser navigation buttons.
  • disabled in the browser, native navigation buttons should be used instead.

A couple of extra fixes that felt natural at that point:

  • I removed all references to backwardUrl and forwardUrl because it looked like they weren't needed anymore.
  • EpubControls also uses the browser history when going back, instead of forcing it to go to books/id
  • Fixed some tedious lint errors in AppRouter

aaronleopold and others added 2 commits March 16, 2023 18:49
- buttons now mimic native browser navigation buttons.
- disabled in the browser, native navigation buttons should be used instead.
const ServerConnectionError = lazily(() => import('./pages/ServerConnectionError'))
const LoginOrClaim = lazily(() => import('./pages/LoginOrClaim'))
const OnBoarding = lazily(() => import('./pages/OnBoarding'))
const Home = lazily(() => import('./pages/Home.js'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed some tedious lint errors in AppRouter

I assume adding .js was part of this? What was the lint?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it was, also for some reason anything but 'js' would trigger an error as well.

Can't remember what the linter was, I'll let you know in the morning it's 11:30pm here and I'm done with code for the day 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, interesting - alright. I was worried those changes wouldn't bundle properly since those files with .js don't technically exist, but locally building it seems to have properly picked it up so 🤷

Can't remember what the linter was, I'll let you know in the morning it's 11:30pm here and I'm done with code for the day 😅

Haha no worries, have a good night!

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.

LGTM, thanks for working on this! I'll wait to merge until CI completes and to find out what those lints were for AppRouter, mostly out of curiosity 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice, with this I can finally get rid of this store, as it won't be used after al/bye-chakra gets merged

@@ -91,7 +91,10 @@ function EpubHeaderControls({
align="flex-start"
>
<ButtonGroup isAttached>
<IconButton variant="ghost" onClick={() => navigate(`/books/${epub.media_entity.id}`)}>
{/* FIXME: This navigate(-3) is effectively going back three times.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for noting this!

@aaronleopold aaronleopold mentioned this pull request Mar 26, 2023
8 tasks
@aaronleopold aaronleopold merged commit 43a77c0 into stumpapp:develop Mar 26, 2023
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