Adding a header partial integration #453 #454

projektir · 2017-09-27T23:08:27Z

Addresses #453.

Giving this an initial stab.

I've tested it and it works: you need to point the theme at the location of your theme folder with the modified header.hbs, otherwise the default one is blank.

This still doesn't address the problem of additional classes/CSS, though...

Let me know what you think!

budziq · 2017-09-28T11:26:02Z

src/renderer/html_handlebars/hbs_renderer.rs

        handlebars.register_template_string(
            "index",
            String::from_utf8(theme.index.clone())?,
        )?;

+        debug!("[*]: Register the header handlebars template");
+        handlebars.register_template_string(


As far as I understand the register_partial would be contextually more appropriate (yes currently these two functions have identical implementation but this can change in the future)

budziq · 2017-09-28T11:27:08Z

src/book/mod.rs

@@ -262,6 +262,10 @@ impl MDBook {
        let mut index = File::create(&themedir.join("index.hbs"))?;
        index.write_all(theme::INDEX)?;

+        // header.hbs
+        let mut header = File::create(&themedir.join("header.hbs"))?;


and when the "header.hbs" is missing?
I understand that this should be handled similarly to "additional.css/js". Hmm We might be interested in extracting some type hbs partials registry.

So this works similar to other /present/ files in the theme, in that there's already a blank header.hbs file and when the user doesn't supply one, the blank one is used, so it does nothing. I don't think I'm clear on what the difference is.

If we want to be more free form we could just let the user specify partials and then they can override the index.hbs itself to use their own partials in their own places (maybe this is one way to address CSS problems).

budziq · 2017-09-28T11:34:56Z

Thanks @projektir ! I think that we might be interested in generalizing the implementation a little and possibly extraction of some hbs blocks from index.hbs to separate files. But we would need to wait on word from @azerupi if he is interested in such feature at all (I know I am :)).

projektir · 2017-09-28T15:28:48Z

I believe @azerupi marked this as Wishlist, so I think they want it! We just need to agree on how general it should be.

azerupi · 2017-09-28T15:53:48Z

Yes, I fully support splitting the index.hbs file into multiple parts that can be overridden independently. We can start with just this and add more partials in a more iterative fashion. :)

This still doesn't address the problem of additional classes/CSS, though...

What is the problem with additional classes / css?

projektir · 2017-09-28T23:17:47Z

@azerupi
So the initial culprit for this was the Rust books which add a warning header. But what they also do is add a CSS class to one of nav elements (i.e., not part of the header), and then add a separate CSS style for that class.

Here's the relevant commit: rust-lang/book@bcb1cc5#diff-ebf1c3b21d73d0e04f32752d0b51ad38

azerupi · 2017-09-29T09:03:56Z

Ok, but what problem does that cause exactly? Is it the fact that they overwrite the index.hbs file to add a class? If that is the problem they can just add custom css rules to the #page-wrapper element. Using the new keys to add additional css and js files it should be fairly trivial.

If that is not the problem I need more clarifications to understand.

projektir · 2017-09-29T17:35:20Z

It means they still need to override the header.hbs directly, which means it still needs to be rebased once in a while to keep it in sync with any changes introduced to mdBook. That's what spurred @Havvy to make this issue originally I believe.

I'm not sure why you're reacting like that. If you think there's no problem that's a good thing, but I don't don't necessarily know your view nor do I have the full picture of what mdBook does, so I don't think it's unreasonable for me to want make sure that the original issue is indeed addressed.

Additional CSS and JS can be used to add a header even without a partial, also, but this is not what happened, so potentially there's a barrier to using those. But, yes, it can be done.

Havvy · 2017-09-29T22:33:11Z

For TRPL, since there's other changes to index.hbs, it'd be okay to leave the CSS thing there. Just minimizing the diff we have to upkeep would be beneficial in and of itself.

For The Reference, we can remove the class, and just add the CSS to the page no matter what - leaving a comment in both the header.hbs and whichever CSS file saying what they are for.

The CSS we use is:

.page-wrapper.has-warning > .nav-chapters {
   /* add height for warning content & margin */
  top: 120px;
}

So if the .nav-chapters could be below whatever content is in header.hbs without having to specify, we wouldn't need the CSS class at all.

projektir · 2017-09-30T00:37:35Z

For TRPL, since there's other changes to index.hbs, it'd be okay to leave the CSS thing there.

Oh, I didn't know that. Yeah that makes sense then.

I'm not sure if I'm quite following the thing with the reference but I guess that's mostly going to be figured out on that side.

If someone wants me to add the other parts to be partials let me know, otherwise we can perhaps close this?

azerupi · 2017-09-30T07:03:04Z

I'm not sure why you're reacting like that. If you think there's no problem that's a good thing

I'm sorry, I think we misunderstood each other. I truly did not understand what the problem was and I want to understand the issue before suggesting any solution. Now that I better understand the problem, I can try to find the best possible solution.

I thought that the problem you were talking about was simply adding some new classes to elements and give them a style. So I was trying to propose an alternate solution for the commit you linked to. Now I see that you want the underlying problem to be solved, namely to not have to override the included .hbs files for simple changes.

I think it is a problem worth solving, so I am going to think about this.

An idea for exploration: It should be possible to extend templates and include the original inside the extended template. At least in other templating languages it is possible if I remember correctly. This would allow users to extend the templates without overwriting them.

projektir · 2017-10-02T03:45:44Z

@azerupi so that sounds like a separate effort. Can we merge this PR for now and close the issue about the header and open a different one, perhaps?

Michael-F-Bryan · 2017-11-16T08:13:26Z

@projektir, other than doing a rebase and resolving any merge conflicts is there anything else you think we need to do before this is ready to merge?

projektir · 2017-11-22T08:56:09Z

Whoops, sorry, didn't notice this update. I'll rebase this up in a day or two.

projektir · 2017-11-25T18:14:20Z

I rebased it, but I'm having trouble replacing the header now... not sure if I am doing something wrong or something else changed with theme replacement...

projektir · 2017-12-01T05:49:47Z

@Michael-F-Bryan
I went back and fiddled with the config and seems like it is working, so this should be good to merge. There's somewhere I wouldn't mind using this, as well.

Michael-F-Bryan · 2017-12-02T05:35:48Z

LGTM 👍

It seems like all mac jobs for travis are blocked indefinitely, but that's most probably just Travis struggling with availability. I'm going to merge this.

Adding a header partial integration rust-lang#453

budziq reviewed Sep 28, 2017

View reviewed changes

Adding a header partial integration #453

32df76d

Michael-F-Bryan merged commit b614b0f into rust-lang:master Dec 2, 2017

projektir deleted the header_partial branch December 2, 2017 06:06

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020

Merge pull request rust-lang#454 from projektir/header_partial

5d52820

Adding a header partial integration rust-lang#453

WofWca mentioned this pull request Oct 11, 2020

header.hbs renders above the top of the HTML document, out of view #1331

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a header partial integration #453 #454

Adding a header partial integration #453 #454

projektir commented Sep 27, 2017 •

edited

Loading

budziq Sep 28, 2017

budziq Sep 28, 2017

projektir Sep 28, 2017

budziq commented Sep 28, 2017

projektir commented Sep 28, 2017

azerupi commented Sep 28, 2017 •

edited

Loading

projektir commented Sep 28, 2017

azerupi commented Sep 29, 2017

projektir commented Sep 29, 2017

Havvy commented Sep 29, 2017

projektir commented Sep 30, 2017

azerupi commented Sep 30, 2017

projektir commented Oct 2, 2017

Michael-F-Bryan commented Nov 16, 2017

projektir commented Nov 22, 2017

projektir commented Nov 25, 2017

projektir commented Dec 1, 2017 •

edited

Loading

Michael-F-Bryan commented Dec 2, 2017

Adding a header partial integration #453 #454

Adding a header partial integration #453 #454

Conversation

projektir commented Sep 27, 2017 • edited Loading

budziq Sep 28, 2017

Choose a reason for hiding this comment

budziq Sep 28, 2017

Choose a reason for hiding this comment

projektir Sep 28, 2017

Choose a reason for hiding this comment

budziq commented Sep 28, 2017

projektir commented Sep 28, 2017

azerupi commented Sep 28, 2017 • edited Loading

projektir commented Sep 28, 2017

azerupi commented Sep 29, 2017

projektir commented Sep 29, 2017

Havvy commented Sep 29, 2017

projektir commented Sep 30, 2017

azerupi commented Sep 30, 2017

projektir commented Oct 2, 2017

Michael-F-Bryan commented Nov 16, 2017

projektir commented Nov 22, 2017

projektir commented Nov 25, 2017

projektir commented Dec 1, 2017 • edited Loading

Michael-F-Bryan commented Dec 2, 2017

projektir commented Sep 27, 2017 •

edited

Loading

azerupi commented Sep 28, 2017 •

edited

Loading

projektir commented Dec 1, 2017 •

edited

Loading