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

Handle input path with regards to custom css #598

Merged
merged 2 commits into from
Jan 30, 2018

Conversation

steveklabnik
Copy link
Member

Before, when someone like the Reference set their extra css as
"theme/reference.css" in their book.toml, this path would be treated as
relative to the invocation of mdbook, and not respect the input path. This
PR modifies these relative paths to do so.

Fixes the build of rust-lang/rust#47753 which
blocks updating rustc to mdbook 0.1

It'd be nice to get a release cut with this fix so we can ship that PR!

Before, when someone like the Reference set their extra css as
"theme/reference.css" in their book.toml, this path would be treated as
relative to the invocation of mdbook, and not respect the input path. This
PR modifies these relative paths to do so.

Fixes the build of rust-lang/rust#47753 which
blocks updating rustc to mdbook 0.1
@steveklabnik
Copy link
Member Author

apparently this breaks links: https://travis-ci.org/rust-lang/rust/builds/334450214#L2554

@Michael-F-Bryan
Copy link
Contributor

Oops, I think this might have slipped in when trying to break the HTML renderer's render() function into smaller chunks. This PR looks like it should fix things though, do you know why the Rust build is still failing link checks?

@steveklabnik
Copy link
Member Author

do you know why the Rust build is still failing link checks?

I suspect that on nested pages, the relative links break. that's my guess though, I haven't dug into the output yet, and am not likely to be able to till tomorrow

@Michael-F-Bryan
Copy link
Contributor

I suspect that on nested pages, the relative links break.

Hmm, that's interesting. I don't know how css loading interacts with the <base> path, but it seems like this isn't the first time we have issues with relative vs absolute paths.

cc #597

@steveklabnik
Copy link
Member Author

I don't know how css loading interacts with the path,

I bet the linkchecker isn't taking <base> into account, and that's the issue.

@Michael-F-Bryan
Copy link
Contributor

Is there anything we can do to deal with that in this PR, or would it be a problem to solve by updating the linkchecker directly?

@steveklabnik
Copy link
Member Author

hm, i just checked linkchecker and it does at least have code in it to handle base tags. gotta dig deeper.

@steveklabnik
Copy link
Member Author

it is just generating a bad link. i was solely looking at the switcher stuff; but it turns out that

    <link rel="stylesheet" href="reference.css">

is being generated rather than

    <link rel="stylesheet" href="theme/reference.css">

@steveklabnik
Copy link
Member Author

this happens because of this code in hbs_renderer

    // Add check to see if there is an additional style
    if !html.additional_css.is_empty() {
        let mut css = Vec::new();
        for style in &html.additional_css {
            match style.strip_prefix(root) {
                Ok(p) => {
                    css.push(p.to_str().expect("Could not convert to str"))
                },
                Err(_) => {
                    css.push(style.file_name()
                                  .expect("File has a file name")
                                  .to_str()
                                  .expect("Could not convert to str"))
                }
            }
        }
        data.insert("additional_css".to_owned(), json!(css));
    }

because the style name is theme/reference.css, this results in a Err(StripPrefixError(())), which means that we push only the file_name, losing the theme bit.

Is the right thing to do to just not push the file_name? gonna give that a try.

the style name is theme/reference.css, this results in a Err(StripPrefixError(())), which means that we push only the file_name, losing the theme bit
@steveklabnik
Copy link
Member Author

okay, so with this latest code, stuff seems to build

@Michael-F-Bryan
Copy link
Contributor

Is the right thing to do to just not push the file_name? gonna give that a try.

I'd say it should push the path (like you've done), not just the file_name.

Let me know when the builds are working again and I'll create another point release.

@steveklabnik
Copy link
Member Author

the failure in the current build isn't related to this stuff; we should be good to :shipit:

@steveklabnik
Copy link
Member Author

yup, all green now!

@Michael-F-Bryan Michael-F-Bryan merged commit 3ba71c5 into master Jan 30, 2018
@steveklabnik
Copy link
Member Author

steveklabnik commented Jan 30, 2018

❤️ any chance of a release any time soon, @Michael-F-Bryan ?

@steveklabnik steveklabnik deleted the fix-theme-copying branch January 30, 2018 14:13
@udoprog
Copy link

udoprog commented Jan 30, 2018

FWIW, additional_css, additional_js, and theme should probably also be RelativePath's seeing as they are serializable. Any absolute path specified in a configuration file would not be portable (see #597).

EDIT: I think this would also mean that the strip_prefix branch that was fixed here would be removed?

@Michael-F-Bryan
Copy link
Contributor

any chance of a release any time soon, @Michael-F-Bryan ?

I'm at work at the moment, but when I get home I'll make another release. Is there anything else mdbook-related that's blocking rust-lang/rust#47753?

@steveklabnik
Copy link
Member Author

Nope! as you can see, the latest commit uses mdbook master at that particular hash. 👍 ❤️

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
* Handle input path with regards to custom css

Before, when someone like the Reference set their extra css as
"theme/reference.css" in their book.toml, this path would be treated as
relative to the invocation of mdbook, and not respect the input path. This
PR modifies these relative paths to do so.

Fixes the build of rust-lang/rust#47753 which
blocks updating rustc to mdbook 0.1

* don't use file-name

the style name is theme/reference.css, this results in a Err(StripPrefixError(())), which means that we push only the file_name, losing the theme bit
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.

3 participants