azu / gitbook-plugin-include-codeblock

GitBook plugin for including file
Other
40 stars 25 forks source link

Feature: add optional title #19

Closed gdolle closed 8 years ago

gdolle commented 8 years ago

Implement a new option title to add a title to source code (see discussion issue #3) (Should be disable by default. It would be a workaround for issue #3).

Usage:

command description
[import, title:<the title>](path/to/code.ext) Display <the title> above source code.
[import, title](path/to/code.ext) Display file name above source code

NB: Specifying a title should create an anchor.

Example:


[example]()

// source code


[file.js]()

// source code


// source code

No title => no anchor in the last case (similar behavior as LaTeX, no caption/label, no references which seems consistent).

azu commented 8 years ago

[import, title](path/to/code.ext) Display file name above source code

Ah, I have overlooked that(title attribute only pattern). I've always used following pattern.

[import, file.js](path/to/file.js)

it will work as link with title on GitHub(work as fallback).

import, file.js

However, title attribute only pattern is bad experience on GitHub.

[import, title](path/to/file.js)

will be Screenshot on GitHub.

The user use [import, title:file.js](path/to/file.js) instead of [import, title](path/to/file.js) for avoiding the bad result.

import, title:file.js

[import, title:<the title>](path/to/code.ext) Display above source code.

On the one hand, I agree with title: command support.


My suggestion

[import, title](path/to/code.ext)

@gdolle's proposal describe that code.ext is a title. I think that it is unexpected things for plugin user. ( Markdown has a title like [label](https://www.google.com "Google's Homepage"))

If the plugin user want to show title, always use title:code.ext command pattern.

So, We no need to implement title attribute only pattern.

@gdolle What do you think about this?

gdolle commented 8 years ago
  • Not support title attribute only pattern.
  • Add title:"<title>" command pattern support.
  • Change default behavior: don't show title and show only imported code.

@azu Ok for me!

Concerning title + anchors, we return currently for markdown

<a name"[label]"=>[title]</a>

I figure that we should also put the [label] equal to the [title] to enable references [the label name](#label) to slice of code instead for the filename. Ideally, I think we should add label attribute to let the user set constant labels (and then avoid to redefine a label each time the title change).

We should also add differentiate the asciidoc format case and put instead

.title
[[label]]

On asciidoc page, you could put references using <<label,the label name>>


So new suggestion

Does that seems ok for you ?

azu commented 8 years ago

@gdolle

Add label attribute import,title:"",label:"". If the label is not defined, but the title is, then we set the = Differentiate the asciidoc case

Ok. It it reasonable.

One concern, I confused naming of label:. Asciidoctor called xreflabel.

Another naming:

Implementation Idea

(:memo: This code is ideal)

I think, It is possible to implement by following way.

First Parse the label of markdown: (label means import,title:"<thetitle>",label:"<thelabel>")

[import,title:"<thetitle>",label:"<thelabel>"](path/to/file.ext).

parseVariablesFromLabel(label) parse the label and return key-value object.

const label = `import,title:"<thetitle>",label:"<thelabel>"`;
const keyValueObject = parseVariablesFromLabel(label);
console.log(keyValueObject);
/*
{
    title: "<thetitle>"
    label:"<thelabel>"
}
*/

parseVariablesFromLabel() parse [ <any-key>:<any-value>, <any-key>:<any-value>] and return key-value object.

Another parseVariablesFromLabel example:

const label = `import:markerName, title:"<title>", id:"<id>"`;
const keyValueObject = parseVariablesFromLabel(label);
console.log(keyValueObject);
/*
{
    import: "markerName", // maybe unnecessary...
    title: "<title>",
    id:"<id>"
}
*/

generateEmbedCode function render the template with the keyValueObject.

It means that generateEmbedCode use template engine like Handlebars.

// https://github.com/sindresorhus/object-assign
const objectAssign = require('object-assign');
// https://www.npmjs.com/package/handlebars
const Handlebars = require("handlebars")

function generateEmbedCode(lang, fileName, originalPath, content) {
    // TODO: separated module!
    const label = `import,title:"<thetitle>",label:"<thelabel>"`;
    const keyValueObject = parseVariablesFromLabel(label);
    // merge objects 
    // if keyValueObject has `lang` key, that is overwrited by `lang` of right.
    const context = objectAssign({}, keyValueObject, { lang, fileName, originalPath, content });

    // TODO: separated template file
    // if has the title, add anchor link
    const source = `{{#if title}}
    > <a name="${label}" href="{{originalPath}}">{{title}}</a>
     {{/if}}
    \`\`\` {{lang}}
    {{content}}
    \`\`\``
    };
    const template = Handlebars.compile(source);
    // compile with data
    const output = template(context);
    // => markdown strings
    return output;
}
gdolle commented 8 years ago

I agree with you this implementation sounds far better to handle eventual future key:value attributes.

One concern, I confused naming of label:

Yes in fact I based this naming on LaTeX. But we can, as you suggested, add several naming. Also, for the title attribite, I think we could add the caption naming for people that are used to LaTeX!

I think maybe we should force also the attribute "quotes" for the import attribute import:"marker" or import:"1-2" to be consistent.

azu commented 8 years ago

I like following syntax:

[import, title:<title>, id:<id>](./path/to/file.js)

it will be

<a id="id" href="./path/to/file.js">title</a>

:memo: id attribute also work as anchor name.

import:"1-2"

I think that slice code format should not be changed. (parseVariablesFromLabel() will just ignore this) Number format is natural and backward compatibility... But, I have not strong strong opinion about this.

gdolle commented 8 years ago

[import, title:<title>, id:<id>]() With this format, I see only a problem with parsing attributes if the title contains punctuation especially ',' ':' which sounds reasonable to me (depending on language rules).

import:"1-2" I think that slice code format should not be changed.

I'm ok for the range. We can add put "" optional only for import|include:markers for backward compatibility ?

azu commented 8 years ago

[import, title:<title>, id:<id>]()

Ah, I mistake. I have intended to write this.

[import, title:"<title>", id:"<id>"](./path/to/file.js)

azu commented 8 years ago

We can add put "" optional only for import|include:markers for backward compatibility ?

markers feature is young. I'm ok with forcing quote. (I think that import:"markerName" is natural, because markerName is always string type)

gdolle commented 8 years ago

Ah, I mistake. I have intended to write this.

Tired after a long day :smile:

markers feature is young. I'm ok with forcing quote.

Ok!

gdolle commented 8 years ago

@azu I've updated the pull-request #20 code to be reviewed.

In the last version, issue #18 is fixed, for use of quotes "" to markers. The format is:

[include:"markers",title:"thetitle",id:"theid",lang-typescript](/path/to/code.ext)

I propose also another idea: to introduce code numbering in [0,Ncode] which apply to the whole book. A unique number is added to the code only if a title is set.

See this example.

2016-05-07-192356_1920x1080_scrot

I think this is very useful for printed book.

You can cite the code in asciidoc using <<theid>>, it replaces it by the code number. If the user want to pass a specific text, he can use <<theid,the ref text>>. Markdown does not have auto replacement (to my knowledge), user can reference the code using [the ref text](#theid).