Some love for the rustbook (engine) #26216
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
I want to add some icons. But eventually, more could be used: search, github link, ... They have a CDN so I don't need to include the icons in the rust repo, I can just link to their CDN (like I do for the Open Sans google font) |
|
it's important that docs render well when not-online, so I am tempted to say no to FontAwesome. There are some that already argue our existing fonts are too much... |
|
Nice improvements! A few thoughts:
|
|
Oh that's a pity, it would have added a nice touch instead of menus made of words. I don't see their point though, since everything is packed with the docs it doesn't matter much how much font's we use. The only argument I could understand is that it increases page load (but that's only relevant for the online version) and you can improve that by using CDN's + local fallback. Is there another argument against it? @alexcrichton I used Open Sans from google fonts so it's not added to the repo. It does show up when you browse locally and have a an internet connection. But if it doesn't load it's no problem at all because you fall back on the old font ;)
I think I can :) I will post a link when it's done |
|
@alexcrichton @steveklabnik here is the link: http://www.mathieudavid.org/test/rustbook/patch1/ I am working on a version to change font style at the moment I have included FontAwesome through CDN link I will add a local fallback to use words instead of icons when it doesn't load (for offline support). Once done I will put it online for you to review. Update: For the variable font-sizes, it's a little trickier than I thought. I have to convert all the font-size to What do you think? Update 2: Just to be clear, this is not in the PR (yet) |
|
I would need some help with separating the css and js from rust. At the moment it is hardcoded in a Rust string and not very maintainable. But I'm not sure what I have to modify. Also it would be super convenient to add a make target like |
|
I have effectively isolated the css and js from the rust code. I put them in a static folder. Since It seems to behave correctly but I would feel a hell of a lot more confident if you guys could double check it ;) Edit: This commit has a little downside: now when you change only the css or js, it doesn't rebuild because: |
|
☔ The latest upstream changes (presumably #26037) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@steveklabnik @alexcrichton any update on this? :) |
|
|
||
| }); | ||
| </script> | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
This line looks like junk leftover from a merge.
There was a problem hiding this comment.
Oh, yes my bad. I completely overlooked that. Correcting it right away :)
…re beautiful with more space and colors, added some style to the code blocks
…n links to add bigger margin
|
@nhowell The rebase worked, I think it's all good now. Thanks for your help ! |
|
I think this change is overall good, but still am opposed to Yet More Web Fonts. We get a lot of complaints about our existing usage, I'd rather be conservative for now. Other than that, r=me |
|
@steveklabnik Ok, I will remove the font then :) But I would like to explain why (in my opinion) it has only advantages. Could this maybe be discussed on the internal forum, to see what others think about it?
And I think it's easier on the eye, but that's a matter of personal taste ;) Was there a change with |
A lot of people complain about our existing web font usage. I'd prefer to keep that usage level at about the same until we see if maybe someday we remove it entirely. We really need some help from some designer people :( I don't know about any changes to |
|
@bors: r+ |
|
📌 Commit 003c3ea has been approved by |
|
💔 Test failed - auto-mac-64-opt |
|
Okay, I have no idea here. @alexcrichton @brson do either of you know why
this would start failing?
|
|
My guess is that this command to copy static assets from a relative directory of cwd is failing. Does this patch work with out-of-tree builds? |
|
Thanks for the rustbook love. It could be a swell tool. |
What would cause it to fail? It works fine on my machine.. 😒 |
|
It's creating a path from the current working directory to somewhere in the source directory. This should be fine if building from the source directory, but if building out of tree (running configure from somewhere other than the source directory) it will produce the wrong path. Again just a guess. |
|
That makes sense. Unfortunately I don't see any variable that points to the root directory and I cant use a relative path from Is there something I could use that will always point to the right directory? |
|
@azerupi I'd suggest following the example here and using |
|
Sorry, didn't mean to close. |
|
⌛ Testing commit 003c3ea with merge 358f1dd... |
|
💔 Test failed - auto-mac-64-opt |
|
☔ The latest upstream changes (presumably #27066) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@azerupi what's your status? |
Yes sorry for the wait, I have been distracted by other things (studies). |
|
I think it's good now, can someone review it? |
|
@bors: r+ |
|
📌 Commit f6e9240 has been approved by |
|
Looks good, let's see what bors says :) |
So I have tried to improve the rustbook engine:
- The sidebar now looks a lot more like gitbook (I thinks it cleaner)
- Added the Open Sans font, in my opinion more readable for prolonged periods of time
- Changed the style for code blocks a little
I encountered 1 problem. In `build.rs` I added this google font url (I commented out the non-relevant parts for clarity)
```rust
let rustdoc_args: &[String] = &[
//"".to_string(),
//preprocessed_path.display().to_string(),
//format!("-o{}", out_path.display()),
//format!("--html-before-content={}", prelude.display()),
//format!("--html-after-content={}", postlude.display()),
//format!("--markdown-playground-url=http://play.rust-lang.org"),
//format!("--markdown-css={}", item.path_to_root.join("rust-book.css").display()),
format!("--markdown-css=http://fonts.googleapis.com/css?family=Open+Sans:400italic,700italic,400,700"),
//"--markdown-no-toc".to_string(),
];
```
As you can see, I had to escape `=` with `=` because the string would get truncated if I didn't. Is that normal behaviour? Is that for security measures? If it is, isn't it a little weak if you can circumvent it by escaped characters? I don't know the reason behind, but I thought it was at least worth mentioning :)
Take your time for this PR, I still want to add multiple improvements:
- Like gitbook, possibility to change font by user
- Put `css` and `js` in their respective files (not hardcoded in rust)
- button to hide sidebar
- ...
So I'm not in a hurry to get this merged ;) But if you think it's good enough to be merged, go ahead. I will make another PR when I have other improvements.
In the image below is a screen of the improvements
|
Great ! Finally, thanks to everyone for your patience :) |
So I have tried to improve the rustbook engine:
I encountered 1 problem. In
build.rsI added this google font url (I commented out the non-relevant parts for clarity)As you can see, I had to escape
=with=because the string would get truncated if I didn't. Is that normal behaviour? Is that for security measures? If it is, isn't it a little weak if you can circumvent it by escaped characters? I don't know the reason behind, but I thought it was at least worth mentioning :)Take your time for this PR, I still want to add multiple improvements:
cssandjsin their respective files (not hardcoded in rust)So I'm not in a hurry to get this merged ;) But if you think it's good enough to be merged, go ahead. I will make another PR when I have other improvements.
In the image below is a screen of the improvements