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

Make pathname stay when version changes in docs #263

Merged

Conversation

nichtsam
Copy link
Contributor

@nichtsam nichtsam commented May 14, 2024

Currently switching branches or version throws users back to the index page.
I noticed that VersionWarningMessage preserves the path, so I just copied the logic over.

@brookslybrand
Copy link
Contributor

Hey @nichtsam this is great! Thanks for adding this

I noticed that there's one small bug. I think once this is fixed I'm good to merge this in

undefined_route.mov

@nichtsam
Copy link
Contributor Author

nichtsam commented May 22, 2024

Hey @nichtsam this is great! Thanks for adding this

I noticed that there's one small bug. I think once this is fixed I'm good to merge this in

undefined_route.mov

I actually noticed the same thing, but the 'VersionWarningMessage' does this too, and I wasn't sure how to tackle this nicely.

Throwing user to the main page when it's 404 seems a bit rude and unclear.
I thought at least 404 gives an idea to users that what they were looking doesn't exist.

@nichtsam
Copy link
Contributor Author

Hey @nichtsam this is great! Thanks for adding this
I noticed that there's one small bug. I think once this is fixed I'm good to merge this in

undefined_route.mov

I actually noticed the same thing, but the 'VersionWarningMessage' does this too, and I wasn't sure how to tackle this nicely.

Throwing user to the main page when it's 404 seems a bit rude and unclear. I thought at least 404 gives an idea to users that what they were looking doesn't exist.

Ah ok I didn't realize the bug in the url! 🤡
I'll look into this.

@brookslybrand
Copy link
Contributor

Ah ok I didn't realize the bug in the url! 🤡 I'll look into this.

Haha sorry if I didn't make that clear. I imagine it's an easy fix, but ping me if it's not (or when you fix it!). Excited to merge 🙂

@nichtsam nichtsam force-pushed the pr/keep-page-when-version-change branch from f76e0fe to 6340628 Compare May 23, 2024 16:18
@nichtsam
Copy link
Contributor Author

nichtsam commented May 23, 2024

Ah ok I didn't realize the bug in the url! 🤡 I'll look into this.

Haha sorry if I didn't make that clear. I imagine it's an easy fix, but ping me if it's not (or when you fix it!). Excited to merge 🙂

@brookslybrand Fixed!
It is an easy fix, I just didn't give the screen recording a good look 😂
I thought it was about the behavior if the doc doesn't exist after a version switch.

Copy link
Contributor

@brookslybrand brookslybrand left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you @nichtsam

@brookslybrand brookslybrand merged commit 7c95f1c into remix-run:main May 23, 2024
2 checks passed
@nichtsam nichtsam deleted the pr/keep-page-when-version-change branch May 23, 2024 16:36
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.

None yet

2 participants