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.

Sign up for GitHub

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

Jump to bottom

Show Progress Bar on Back #1503

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Show Progress Bar on Back #1503

wants to merge 2 commits into from

Conversation

tsmith123
Copy link
Contributor

Fixes #1471

Observations before fixing

  • If a user clicks on a post from the homepage and then clicks back then the progress bar is now shown
  • If a user click on a post and then clicks to somewhere else and then uses back then the progress bar is shown on every page

Attempted Solution
The issue lies with the passing of shallow: true in _app.js.

This PR offers the quick fix of shallow: false but I would assume this has implications elsewhere in the app?

tsmith123
tsmith123 commented Oct 20, 2024
View reviewed changes
pages/_app.js
// (2) is not possible while intercepting nav with beforePopState
router.replace({
pathname: router.pathname,
query: { ...router.query, nodata: true }
}, router.asPath, { ...router.options, shallow: true }).catch((e) => {
}, router.asPath, { ...router.options, shallow: false }).catch((e) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This probably has implications elsewhere in the app?

Copy link
Member

Choose a reason for hiding this comment

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

yes this will cause the data to load twice on every page visit. no go with this change.

@tsmith123 tsmith123 mentioned this pull request Oct 20, 2024
Open
huumn
huumn requested changes Oct 20, 2024
View reviewed changes
Copy link
Member

@huumn huumn left a comment

Choose a reason for hiding this comment

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

Given that shallow is intentional, I think you'll want to make nprogressStart and nprogessDone make an exception for back navigation if possible rather than changing the behavior of every router.replace operation.

@tsmith123
Copy link
Contributor Author

Thanks. The current behaviour is very strange though... if you go to a post from the homepage and then go to your profile page then click back twice it will show the progress bar for both backs.

@huumn
Copy link
Member

huumn commented Oct 21, 2024

Are you saying we shouldn't have progress bars on back nav? Maybe this is the wrong solution to the right problem.

The problem we have is that nextjs wants to hit the server on back nav because we SSR content. Ideally nextjs wouldn't do that, but at least when this useEffect was written (probably nextjs v10) that wasn't possible.

@tsmith123
Copy link
Contributor Author

tsmith123 commented Oct 21, 2024 •
edited
Loading

My last comment was just to say that if you navigate deeper into the app and then use the back button then you will see a progress bar. But if you just go to a post and then back then you won't - that's the strange behaviour.

I did look into router.back() when I first started looking at the issue but it doesn't offer a shallow option I'm afraid. I would need to see if the "hack" you have in _app.js is still required.

@huumn huumn marked this pull request as draft November 2, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huumn huumn huumn requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Progress bar for back button
2 participants
@tsmith123 @huumn