-
Notifications
You must be signed in to change notification settings - Fork 212
The rest of Tera #879
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
The rest of Tera #879
Conversation
tera-templates/rustdoc/page.html
Outdated
<!DOCTYPE html> | ||
<html lang="en"> | ||
|
||
<head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this use a new base instead of reusing base.html
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
base.html
injects a lot of extra style stuff for rustdoc pages, this was the simplest solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a screenshot of viewing sources for rust files, not just for directories.
{# If the crate has a homepage, show it #} | ||
{%- if details.homepage_url -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give a screenshot of what the details
page looks like when details is None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The homepage link just isn't displayed, Tera treats None
/null
as a falsy value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into trying to unify the details dropdown of crate/details.html and rustdoc/navigation.html but I'm nervous to try since |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a bit out of scope for this PR
I missed that this code was already currently duplicated. Yes, in that case this is all fine.
{# If the crate has a homepage, show it #} | ||
{%- if details.homepage_url -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.set_true("javascript_highlightjs") | ||
.set_true("package_navigation_crate_tab") | ||
.to_resp("crate_details") | ||
CrateDetailsPage { details }.into_response(req) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The more I look at this, the more I think this Option
is not correct. If it's None
, that means that no crate exists for the (name, version) pair:
docs.rs/src/web/crate_details.rs
Line 107 in 6a53a71
return None; |
So we should either
unwrap()
because we've guaranteed that this crate exists, or return a 404. I'm ok with making this a follow-up PR. The good news is this means the templates never have to handle the case when details
is None.
This PR is split into commits, that's by far the best way to review it
Commit 1: Builds
Before
After
Commit 2: Errors
This commit looks larger than it actually is since I had to update the call site of every
ctry!
andcexpect!
invocationBefore
After
Error message is now the page description
Commit 3: Crate Details
Before
After
The builds page is now always displayed
Commit 4: Source
Before
After
Now there's a few more icons used
Commit 5: Rustdoc
Before
After
Commit 6: Release Activity
Before
After
Commit 7: Build Queue
Before
After
cc @antifuchs
Closes #777, #740 and #786