Skip to content

Remove unnecessary redirect #549

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

Open
jyn514 opened this issue Jan 7, 2020 · 6 comments
Open

Remove unnecessary redirect #549

jyn514 opened this issue Jan 7, 2020 · 6 comments
Labels
A-frontend Area: Web frontend E-easy Effort: Should be easy to implement and would make a good first PR mentor This has instructions for getting started P-low Low priority issues

Comments

@jyn514
Copy link
Member

jyn514 commented Jan 7, 2020

Once #534 gets merged, the 'Platform' dropdown will link to the default build /:crate/:version/:platform, which will immediately redirect to /:crate/:version. It would be nice to link to /:crate/:version directly for the default platform.

@jyn514
Copy link
Member Author

jyn514 commented Jan 7, 2020

Here's some half-working code (goes in src/web/rustdoc.rs):

    let target_links = {
        use std::collections::HashMap;
        let mut map = HashMap::new();
        for target in &crate_details.doc_targets {
            match target {
                Json::String(target) => {
                    if target == &crate_details.metadata.default_target {
                        map.insert(target, "".to_owned());
                    } else {
                        let path = format!("/{}", target);
                        map.insert(target, path);
                    }
                }
                _ => log::warn!("unknown JSON type for doc_target: {}", target),
            }
        }
        map
    };

@jyn514 jyn514 added the mentor This has instructions for getting started label Jan 7, 2020
@jyn514
Copy link
Member Author

jyn514 commented Jan 7, 2020

To implement this:

  1. Add a target_links field to CrateDetails
  2. Fill out that field using the code above (or your own code, I wrote this in a rush).
  3. Use those links in the template instead of always using the {{this}} variable.

@jyn514 jyn514 added E-easy Effort: Should be easy to implement and would make a good first PR A-frontend Area: Web frontend P-low Low priority issues and removed minor labels Jun 27, 2020
@shorsher
Copy link
Contributor

shorsher commented Aug 25, 2020

I'll take a look at this, thanks for all the work outlining what needs to be done. Is double clicking on the platform drop down supposed to automatically redirect you? Right now it does not.

@jyn514
Copy link
Member Author

jyn514 commented Aug 25, 2020

Is double clicking on the platform drop down supposed to automatically redirect you?

I wouldn't expect it too ... you're suggesting it should take you to the default platform? I'd be ok with that.

@shorsher
Copy link
Contributor

@jyn514 I think I assumed a bit too much when glancing over the issue :). I don't have a preference either way, but I'll add it as part of this PR and we can get opinions from the larger team.

@jyn514
Copy link
Member Author

jyn514 commented Apr 17, 2021

@pietroalbini do you think this is still worth fixing? I kind of like that it's consistent with all the other redirects, it makes it easier to reason about reading the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend Area: Web frontend E-easy Effort: Should be easy to implement and would make a good first PR mentor This has instructions for getting started P-low Low priority issues
Projects
None yet
Development

No branches or pull requests

2 participants