Skip to content

Book struct #188

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

Closed
wants to merge 58 commits into from
Closed

Book struct #188

wants to merge 58 commits into from

Conversation

gambhiro
Copy link
Contributor

@gambhiro gambhiro commented Dec 20, 2016

This is taking the book-struct branch for #147, rebasing with updates from master, and adding the structs discussed in #146.

The only API-level change is calling the Book chapters frontmatter, mainmatter and backmatter.

The new structs and fields are just placeholders, there is no real effect at the moment.

It compiles, and builds the example book.

I see that it includes the 58 commits since July, I don't know if that's normal.

quornian and others added 30 commits July 16, 2016 10:23
Make sure <ul><li> and </li></ul> are balanced
also fixes rust-lang/book#166 anchors duplication

Thanks @azerupi for mentoring in #156 !
Cheers!
Fixes #156 - anchors are now URI encoded
Added option to configure serve interface and public websocket address.
This is especially important when mdbook is used with CI.
Use bash for the grey background though.
Exit with a nonzero status if we get an error
use macros from the log crate, issue #151
Make line-height for chapter greater than section
… directly to the css file causing them to be overwritten
azerupi and others added 27 commits September 12, 2016 22:50
Fix a bug where files without an extension were not copied
The current section headers are url encoded.  Because of that they
have some funny characters like %20.  We can clean that up by removing
all of the non-word characters before placing them in the anchor.
Switch from rustc_serialize to serde
Fixes #179.

Highlight.js does not apply syntax highlighting to code blocks marked
no-highlight, nohighlight, plain, or text. When it finds blocks of those
languages, it does not add the `hljs` class to those code blocks either.

highlight.css and tomorrow-night.css use the `hljs` class to give code
blocks their backrgound color and text color, and we want that to apply
even if the code doesn't get syntax highlighting markup.

This is a somewhat hacky solution to get just that behavior! After this
commit, code blocks with no-highlight, nohighlight, plain, or text
language set on them will indeed get the hljs colors.
Add hljs class to all code blocks, regardless of highlighting
Use fixed positioning and remove overflow-x's for smoother scrolling …
Copy link
Contributor

@azerupi azerupi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general feeling about this PR is that it tries to add too much configuration options. I think it's best if we first make all the abstractions work (generic renderer, plugins, multi-lang, ...) before we start adding tons of other configuration options.

At this time, I would only go with the bare necessities and make the generic renderer design work well. Once we have done that it will be easier to see how and where we need to add configuration options.

/// TODO Index number of the chapter in its level. This is the Vec index + 1.
index: i32,
/// TODO CSS class that will be added to the page-level wrap div to allow customized chapter styles.
class: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am uncomfortable with the idea of storing a class attribute in the chapter struct. Because the notion of class is very renderer specific. Currently it would be OK, but what happens when we have more renderers? I think chapter should only contain information that is generic over the renderers.

We have already discussed the notion of chapter specific configurations, using YAML headers or another method. I think per-chapter styling would fit better in those configurations.

/// TODO The description of the chapter.
description: String,
/// TODO Index number of the chapter in its level. This is the Vec index + 1.
index: i32,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you plan to do with this field?
If this is just Vec::len + 1, what is the purpose of keeping it in the struct? By duplicating indices you add a synchronisation complexity. For example, if you add a chapter in the middle you need to update all the index fields.

file: PathBuf,

/// TODO The author of the chapter, or the book.
author: Author,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be an Option? Or a Vec because multiple authors are possible?
Can you maybe explain shortly how you see this author property interact with the author we store in the book?

/// TODO The author of the chapter, or the book.
author: Author,
/// TODO The description of the chapter.
description: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be an Option too.

number_format: NumberFormat,
/// TODO Section names for nested Vec<Chapter> structures, such as `[
/// "Part", "Chapter", "Section" ]`
section_names: Vec<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a little too much anticipated. I am not against a high level of customisation, but I want to nail the basics first.

I think we should concentrate on the fundamental necessities at first. Make the renderer abstraction work, make the plugin abstraction work. And after that we can add fine grained customisations. Trying to do everything at once will make it so much more difficult.

@gambhiro
Copy link
Contributor Author

I reckon I better cancel this. I have been doing more work on it since, and it changes of course as one understands it better. I'm putting it all in a branch which I want to take as far as rendering multiple books (i.e. translations) in html and epub.

I will send a note when that's ready to see. It will probably make more sense to read and reason about than half-finished stages.

@gambhiro gambhiro closed this Dec 27, 2016
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.