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

Keep the subpage when going to latest version #454

Merged
merged 8 commits into from
Nov 12, 2019

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 28, 2019

Addresses #200

If the page does not exist on the latest version, searches for a page with that title.

~~TODO: If

  1. Going to a new version that
  2. Deleted the existing module/item and
  3. The user is looking at a platform other than the default,
    then the link will be wrong (it will not include the name of the crate).~~

The solution is to check if req_path[3] is the crate name or the platform. Once this is done, #200 (comment) will be trivial to fix.

This is fixed, see below.

@jyn514
Copy link
Member Author

jyn514 commented Oct 28, 2019

Also I haven't found a test case for an item that was deleted/moved between versions, if anyone knows of one please let me know!

@jyn514
Copy link
Member Author

jyn514 commented Oct 30, 2019

Fixed the bug with a different platform. I've tested the following manually:

  • same item
  • same item with platform
  • renamed item
  • renamed item with platform
  • deleted item
  • deleted item with platform

If anyone has suggestions for how to set up unit tests, I'd be happy to hear them! I'm not sure how to ensure that certain crates exist in the database :/

@jyn514 jyn514 changed the title [WIP] Keep the subpage when going to latest version Keep the subpage when going to latest version Oct 30, 2019
@jyn514
Copy link
Member Author

jyn514 commented Oct 30, 2019

This does not address the following issues:

  • clicking the name of the crate in the top-left does not keep the platform
  • choosing a different platform goes to the root, not to the current item

@jyn514
Copy link
Member Author

jyn514 commented Oct 30, 2019

This is ready for review.

@EverlastingBugstopper
Copy link

This is fantastic, thanks so much @jyn514

Addresses rust-lang#200

If the page does not exist on the latest version, searches for a page
with that title.
Previously, if

1. Going to a new version that
2. Deleted the existing module/item and
3. The user is looking at a platform other than the default,

then the link was wrong.

This is now fixed.
Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

Thanks so much! This is really great functionality for the site, and no doubt it's one of the more requested features we've had. I just have a couple of nits about comments, but otherwise this looks great!

src/web/rustdoc.rs Outdated Show resolved Hide resolved
src/web/rustdoc.rs Show resolved Hide resolved
@jyn514
Copy link
Member Author

jyn514 commented Nov 12, 2019

Added comments as requested :)

Thanks for taking a look at this!

Copy link
Member

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

Thanks so much! Let's get this merged.

@daboross
Copy link

Thanks for doing 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.

None yet

4 participants