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 3 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
24 changes: 24 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,25 @@ <h6 class="always-index" id="level-6-header-outside-headingsearchindex-with-alwa
note.</p>
</li>


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

<li id="fn-2-1" class="footnote-item">null</li>
<li id="fn-2-2" class="footnote-item">null</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>
18 changes: 18 additions & 0 deletions packages/core/src/html/includePanelProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,24 @@ 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 matchs = actualContent.match(footnotePattern);
if (matchs) {
yiwen101 marked this conversation as resolved.
Show resolved Hide resolved
const hrefPattern = /href="#(fn-\d+-\d+)"/;
let footnoteNumber = 1;
yiwen101 marked this conversation as resolved.
Show resolved Hide resolved
let toAppend = '<mb-temp-footnotes>';
matchs.forEach((match) => {
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.

footnoteNumber += 1;
actualContent = actualContent.replace(match, replaceTo);
toAppend += `\n<li id="${href}" class="footnote-item">${$('#fn-1-1.footnote-item')!.html()}</li>`;
yiwen101 marked this conversation as resolved.
Show resolved Hide resolved
});
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
38 changes: 38 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,44 @@ 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);
// to make lint happy; if write as array.join, some lines more than 100 characters;
yiwen101 marked this conversation as resolved.
Show resolved Hide resolved
const a = '<div>\n<p>text<trigger for="pop:footnotefn-1-1"><sup class="footnote-ref">';
const b = '<a aria-describedby="footnote-label" href="#fn-1-1">[1]</a></sup></trigger></p>';
const c = '</div><hr class="footnotes-sep">\n<section class="footnotes">\n';
const d = '<ol class="footnotes-list">\n<popover id="pop:footnotefn-1-1">\n';
const e = ' <template #content><div>\n <p>footnote</p>\n\n';
const f = ' </div></template>\n </popover>\n\n<li id="fn-1-1"';
const g = ' class="footnote-item"><p>footnote</p>\n</li>\n\n</ol>\n</section>\n';
const expected = a + b + c + d + e + f + g;

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

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

Expand Down
Loading