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

Debug cannot import footnote from hash #2441

Merged
merged 23 commits into from
Mar 18, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/userGuide/syntax/footnotes.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,6 @@ belong to the previous footnote.
```
</div>
<div id="examples" class="d-none">

1 + 1 = 2 ^[Math]
</div>
30 changes: 30 additions & 0 deletions packages/cli/test/functional/test_site/expected/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,11 @@ <h3 id="testing-site-nav">Testing Site-Nav<a class="fa fa-anchor" href="#testing
Here is an inline note.<trigger for="pop:footnotefn-1-3"><sup class="footnote-ref"><a aria-describedby="footnote-label" href="#fn-1-3">[3]</a></sup></trigger>
</p>
</div>
<p><strong>Test include footnotes from hash</strong></p>
<div>
<p>text<trigger for="pop:footnotefn-2-1"><sup class="footnote-ref"><a aria-describedby="footnote-label" href="#fn-2-1">[1]</a></sup></trigger>, text2<trigger for="pop:footnotefn-2-2"><sup class="footnote-ref"><a aria-describedby="footnote-label" href="#fn-2-2">[2]</a></sup></trigger>
</p>
</div>
<p><strong>Nunjucks SetExt</strong></p>
<div> front back </div>
<p>arrayVarItem1</p>
Expand Down Expand Up @@ -789,6 +794,31 @@ <h6 class="always-index" id="level-6-header-outside-headingsearchindex-with-alwa
note.</p>
</li>


<popover id="pop:footnotefn-2-1">
<template #content>
<div>
<p>footnote1</p>

</div>
</template>
</popover>
<popover id="pop:footnotefn-2-2">
<template #content>
<div>
<p>footnote2</p>

</div>
</template>
</popover>

<li id="fn-2-1" class="footnote-item">
<p>footnote1</p>
</li>
<li id="fn-2-2" class="footnote-item">
<p>footnote2</p>
</li>

</ol>
</section>

Expand Down

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions packages/cli/test/functional/test_site/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ Hence, the contained markdown should be parsed and output as is, without any par

<include src="testFootnotes.md" />

**Test include footnotes from hash**

<include src="testHashFootnotes.md#import" />

**Nunjucks SetExt**

{% ext externalVar = "_markbind/variable.json" %}
Expand Down
4 changes: 4 additions & 0 deletions packages/cli/test/functional/test_site/testHashFootnotes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div id="import">

text^[footnote1], text2^[footnote2]
</div>
29 changes: 28 additions & 1 deletion packages/core/src/html/includePanelProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,15 @@ function _deleteIncludeAttributes(node: MbNode) {
delete node.attribs.omitFrontmatter;
}

function buildGetNextFootnodeNumber() {
let footnoteNumber = 1;
yiwen101 marked this conversation as resolved.
Show resolved Hide resolved
function defaultGetFootnoteNumber() {
yiwen101 marked this conversation as resolved.
Show resolved Hide resolved
footnoteNumber += 1;
return footnoteNumber - 1;
}
return defaultGetFootnoteNumber;
}

/**
* PreProcesses includes.
* Replaces it with an error node if the specified src is invalid,
Expand All @@ -173,7 +182,8 @@ function _deleteIncludeAttributes(node: MbNode) {
export function processInclude(node: MbNode, context: Context, pageSources: PageSources,
variableProcessor: VariableProcessor, renderMd: (text: string) => string,
renderMdInline: (text: string) => string,
config: Record<string, any>): Context {
config: Record<string, any>,
getNextFootnodeNumber: () => number = buildGetNextFootnodeNumber()): Context {
if (_.isEmpty(node.attribs.src)) {
const error = new Error(`Empty src attribute in include in: ${context.cwf}`);
logger.error(error);
Expand Down Expand Up @@ -230,6 +240,23 @@ export function processInclude(node: MbNode, context: Context, pageSources: Page
const $ = cheerio.load(actualContent);
const actualContentOrNull = $(hash).html();
actualContent = actualContentOrNull || '';
if (actualContent !== '') {
const footnotePattern = /<a aria-describedby="footnote-label" href="#fn-\d+-\d+">\[\d+\]/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

oh also small thing but i was wondering if using cheerio here makes more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emm, but is not to use cheerio, we need the exact id (eg: #${href}.footnote-item)? So we still need to pattern matching (and also to replace the index in the original content).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I get that cheerio will do the same thing but I think its cleaner to use cheerio to select out and append. In that case, we can use cheerio node to do the modifications below as well. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yucheng11122017
Thank you for the clarification; I have explored cheerio apis and made the requested changes. Following is a quick snapshot:
Screenshot 2024-03-14 at 23 47 04

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(console.log on last line was removed in later commits)

const matches = actualContent.match(footnotePattern);
if (matches) {
const hrefPattern = /href="#(fn-\d+-\d+)"/;
let toAppend = '<mb-temp-footnotes>';
matches.forEach((match) => {
const footnoteNumber = getNextFootnodeNumber();
const href = match.match(hrefPattern)![1];
const replaceTo = `<a aria-describedby="footnote-label" href="#${href}">[${footnoteNumber}]`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it necessary to do this replace here? Why are you changing the number for the footnote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have improved this with the following changes:
Screenshot 2024-03-14 at 16 54 05

where the default value is:
Screenshot 2024-03-14 at 16 54 00

last modification:
Screenshot 2024-03-14 at 16 54 13

Copy link
Contributor Author

@yiwen101 yiwen101 Mar 14, 2024

Choose a reason for hiding this comment

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

Motivation being, as this initially is to fix the error in display footnote in the user doc, I decided to be defensive and do the best (without going beyond the scope and making changes outside the hash part) to strive for making importing footnote from a single hash look good.

actualContent = actualContent.replace(match, replaceTo);
toAppend += `\n<li id="${href}" class="footnote-item">${$(`#${href}.footnote-item`)!.html()}</li>`;
});
toAppend += '\n</mb-temp-footnotes>';
actualContent += toAppend;
}
}

if (actualContentOrNull === null && !isOptional) {
const error = new Error(`No such segment '${hash}' in file: ${actualFilePath}\n`
Expand Down
47 changes: 47 additions & 0 deletions packages/core/test/unit/html/includePanelProcessor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,53 @@ test('includeFile replaces <include src="doesNotExist.md" optional> with empty <
expect(result).toEqual(expected);
});

test('includeFile import footnote from hash', async () => {
const indexPath = path.resolve('index.md');

const index = [
'<include src="include.md#exists"/>',
'',
].join('\n');

const include = [
'<div id="exists">',
'',
'text^[footnote]',
'</div>',
'',
].join('\n');

const json = {
'index.md': index,
'include.md': include,
};

fs.vol.fromJSON(json, '');

const nodeProcessor = getNewDefaultNodeProcessor();
const result = await nodeProcessor.process(indexPath, index);
const expected = '<div>\n'
+ '<p>text<trigger for="pop:footnotefn-1-1"><sup class="footnote-ref"><a aria-describedby="'
+ 'footnote-label" href="#fn-1-1">[1]</a></sup>'
+ '</trigger></p></div><hr class="footnotes-sep">\n'
+ '<section class="footnotes">\n'
+ '<ol class="footnotes-list">\n'
+ '<popover id="pop:footnotefn-1-1">\n'
+ ' <template #content><div>\n'
+ ' <p>footnote</p>\n'
+ '\n'
+ ' </div></template>\n'
+ ' </popover>\n'
+ '\n'
+ '<li id="fn-1-1" class="footnote-item"><p>footnote</p>\n'
+ '</li>\n'
+ '\n'
+ '</ol>\n'
+ '</section>\n';

expect(result).toEqual(expected);
});

test('includeFile replaces <include src="include.md#exists"> with <div>', async () => {
const indexPath = path.resolve('index.md');

Expand Down
Loading