Fixed some issue.

[cyrilis]

Made some changes to fix #34 , #33 ,#32 ,#27

[pedrosanta]

Probably here we can bundle this into a single if and remove the console.log, like so:

content.href = if content.filename.match(/\.xhtml$/) then content.filename else "#{content.filename}.xhtml"
content.filePath = path.resolve self.uuid, "./OEBPS/#{content.href}"

[pedrosanta]

Hi @cyrilis,

I've just reviewed the latest additions to the 'API' for the config options object, and I think we should come up with a more 'robust' option for the beforeToc functionality.

My biggest concern, is how, somehow this structure allows to have mixed contents with this property, like so:

contents: [
  {
    beforeToc: true,
    (...)
  },

  {
    beforeToc: false,
    (...)
  },

  {
    beforeToc: true,
    (...)
  },

  (...)
]

In which in this case, the library would be somehow 'moving' contents around and this may not be completely clear to the user. It also, can generate some discrepancies on our current EPUB 2 TOC XHTML template - I'll point them out in detail.

Anyway, I've thought about it and I propose an option/'API' that will make it more clear to the user, IHMO, by:

• Having the contents option accept either a simple array of contents (in this case making all contents after TOC) or an object with two properties contents.beforeToc and contents.afterToc, which would be arrays of contents;

options.contents: [
  (... array of contents, all after TOC)
]

// OR

options.contents = {
  beforeToc: [
    (... array of 'front matter' contents)
  ],
  afterToc: [
    (... array of contents)
  ]
}

• On the lib code, we could easily 'normalize' these two options, by processing/checking options.contents type and if it's a Array, convert it to object format and place the contents on the afterToc array;
• To prevent some 'weird' states hard to handle - having beforeToc contents that have excludeFromToc as false, the TOC would show them on the structure and then show the remaining contents, without generating a TOC entry/generating a skip - for the sake of consistency I would say all beforeToc contents should be excluded from the TOC, either:
  • As a default behaviour to have these with excludeFromToc as true;
  • Or not define or ignore the excludeFromToc option from the beforeToc contents;

Anyway, I can put this proposal out on a PR and we can discuss it further there. (Also gonna point some issues I think may occur.)

Cheers!

P

[pedrosanta]

@cyrilis Here, with current 'API', if the author doesn't take the care to have all beforeToc contents first in the array, having instead mixed true/false contents along the array, the order of this EPUB 2 TOC will be different from the one on the spine, NCX and the book. Just another issue I found by having the property beforeToc more permissive.

If we had two contents.beforeToc and contents.afterToc arrays, that would be easier/clearer to enforce the consistency, IMO.

[pedrosanta]

@cyrilis Is it 'normal' to have the actual TOC as a TOC entry? Seems a bit confusing to me. I'm gonna ask the book nerds at Reedsy about this question, they surely know this out. 🙂

[cyrilis]

@pedrosanta No, it's not normal to have a TOC content. but beforeToc and afterToc may make API more complicated.

How about make TOC an option content in the data array? like this:

content: [{
        title: "About the author",
        author: "John Doe",
        data: "<h2>Charles Lutwidge Dodgson</h2>" +
            "<div lang=\"en\">Better known by the pen name Lewis...</div>"
    }, {
        title: "Copyright",
        data: "<h2>Copyright</h2>..."
    }, {
        type: "TOC" // => TOC here, will auto generate as normal content data.
    }, {
        title: "Down the Rabbit Hole",
        data: "<p>Alice was beginning to get very tired...</p>"
    }, {
        ...
    }
    ...
]

We can parse this data array, if found data.type == 'TOC', replace it with a generated TOC html data. if not found TOC type, no TOC will insert into ebook, so someone can make their own TOC content, which can locate chapters with the filename attribute.

What do you think? 🙂

[pedrosanta]

Hi @cyrilis, yap I think that proposal of yours seems a bit better and clear to me than what we currently have. I would just suggest for the type to be lowercase, like so type: 'toc'. 🙂

[pedrosanta]

Also, since we're on the subject, how about moving the TOC template property to this content, like so:

(...),
{
  type: 'toc',
  template: 'my-toc-template.xhtml.ejs'
},
(...)

And if no template is provided, lib uses its template.

Just a thought. Wdyt? Cheers!

[pedrosanta]

Also a small note @cyrilis, about TOC and pre-TOC content. So I've asked around our book design experts about (1) having the actual TOC entry on the TOC and (2) any reason to have pre-TOC contents listed on TOC.

And for the first one, it's not common to have the TOC entry on the TOC, nor for second one is common to have pre-TOC contents listed on the TOC. [1][2]

And, seems to me, IMO, that the lib should try to follow the best practices as closely as possible, so I think we could aim our templates to have this default behaviour. 🙂

Wdyt?

[1] Reedsy Blog - Anatomy of a Book: Front Matter, Body and Back Matter
[2] Wikipedia - Book Design: Front matter

[cyrilis]

Yeah, sounds good to me.

And the params passed to the template.xhtml.ejs file should be documented
in the Readme.
On Thu, 17 Nov 2016 at 7:41 PM Pedro Machado Santa notifications@github.com
wrote:

@pedrosanta commented on this pull request.

In templates/epub3/toc.xhtml.ejs
#35:

     <li class="table-of-content">

•        <a href="<%= content.href %>"><%= (content.title || "Chapter "+ (1+index)) %><% if(content.author.length){ %> - <small class="toc-author"><%= content.author.join(",") %></small><% }%><% if(content.url){ %><span class="toc-link"><%= content.url %></span><% }%></a>
  

•        <a href="toc.xhtml">\- <%= tocTitle %> -</a>
  

Also, since we're on the subject, how about moving the TOC template
property to this content, like so:

(...),
{
type: 'toc',
template: 'my-toc-template.xhtml.ejs'
},
(...)

Just a thought. Wdyt? Cheers!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABv8zWNdLN4yub9PDhjRiCfSganf9FMvks5q_D1PgaJpZM4Kskzz
.

[pedrosanta]

@cyrilis Yep, I mean, I think we're passing the full options object to the template... we could suggest users to use that as a carrier, and continue to provide it to the template. That way users would have plenty of flexibility to build their own templates.

Like I said, at Reedsy, I was able to fully customise the TOC and book just by leveraging the custom templates and using like, internal options for that customisation, like options._tocIndex and others.

Perhaps, in that regard, we could have or suggest a 'userspace' property to place these variables if they wish to extend/customise it further, something like options.locals = { ... }, to avoid eventual collisions with any lib internal variable.