Debug cannot import footnote from hash #2441

yiwen101 · 2024-03-03T19:36:52Z

What is the purpose of this pull request?
Resolve #2203 (but #2430 still persists)

Overview of changes:

Anything you'd like to highlight/discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Fix issue of cannot import footnote from hash

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No unrelated changes

currently, when importing from hash, footnote is missing. For example, with

<div id="import">

text^[footnote]
</div>

and

<include src="contents/topic1.md#import"/>

this is rendered

but we expect

This is fixed by using regular pattern to greedily extract and append the missing information.

Please note that #2430 still persists (the index of the imported footnotes may remain be conflicting). This PR only fixes only failure in importing footnote from hash, but did not touch the issue of wrong index.

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

codecov · 2024-03-03T19:39:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.22%. Comparing base (371228d) to head (d555215).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2441      +/-   ##
==========================================
+ Coverage   49.04%   51.22%   +2.17%     
==========================================
  Files         124      124              
  Lines        5262     5281      +19     
  Branches     1118     1121       +3     
==========================================
+ Hits         2581     2705     +124     
+ Misses       2376     2290      -86     
+ Partials      305      286      -19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kaixin-hc

Yay, thanks for the tests!

The issue #2203 also raises the example of the footnote in reader facing features. However, this is not fixed by your PR (is it because the new line isn't there?)

Can you also fix this documentation bug to demonstrate that this feature works?

packages/core/test/unit/html/includePanelProcessor.test.ts

packages/core/src/html/includePanelProcessor.ts

yiwen101 · 2024-03-04T04:19:51Z

Yay, thanks for the tests!

The issue #2203 also raises the example of the footnote in reader facing features. However, this is not fixed by your PR (is it because the new line isn't there?)

Can you also fix this documentation bug to demonstrate that this feature works?

Thanks for the comment; should be fixed in this commit

packages/core/test/unit/html/includePanelProcessor.test.ts

kaixin-hc · 2024-03-04T07:23:36Z

Behavior for footnotes here is definitely unexpected...

yucheng11122017

Thanks for the work @yiwen101!
This is a rather tricky bug to fix so appreciate the work.

I have some concerns about the ordering of the footnote. I understand you aren't trying to address the wrong ordering here but I think this might result in overlaps - not just an issue with wrong ordering. Could you explain why you change the footnote number and why the appended footnote item will has the number 1-1?

Thanks!

packages/core/src/html/includePanelProcessor.ts

yucheng11122017 · 2024-03-04T11:05:54Z

packages/core/src/html/includePanelProcessor.ts

+        let toAppend = '<mb-temp-footnotes>';
+        matches.forEach((match) => {
+          const href = match.match(hrefPattern)![1];
+          const replaceTo = `<a aria-describedby="footnote-label" href="#${href}">[${footnoteNumber}]`;


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

I have improved this with the following changes:

where the default value is:

last modification:

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.

packages/core/test/unit/html/includePanelProcessor.test.ts

yucheng11122017 · 2024-03-04T23:39:04Z

packages/core/src/html/includePanelProcessor.ts

@@ -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;


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

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).

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?

@yucheng11122017
Thank you for the clarification; I have explored cheerio apis and made the requested changes. Following is a quick snapshot:

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

yiwen101 · 2024-03-05T03:42:33Z

Behavior for footnotes here is definitely unexpected...

emm, thanks for pointing out this (and sorry for not examining it closely);will be working on it.

yiwen101 · 2024-03-05T03:50:33Z

Thanks for the work @yiwen101! This is a rather tricky bug to fix so appreciate the work.

I have some concerns about the ordering of the footnote. I understand you aren't trying to address the wrong ordering here but I think this might result in overlaps - not just an issue with wrong ordering. Could you explain why you change the footnote number and why the appended footnote item will has the number 1-1?

Thanks!

Thanks for the careful review.
The the number 1-1 is an unintended bug, thank you for pointing it out!
The footnoteNumber is just a temporary placeHolder; the thinking is that when I/somebody attempt 2430 in the future and add footnoteNumber as an argument to this method, the rest of the code does not need to be changed (wait, just come to me, how to do passing by reference in javascript? nevermind, there must be a way to do it).

yiwen101 · 2024-03-14T09:11:48Z

@kaixin-hc @yucheng11122017
I have made the requested changes, and is ready for review;
Summary of the changes:
1 improve the readability of the test
2 improve the code quality issue of hardcoded footnoteNumer = 1
3 fix bug.
Some additional note on to the third point:
Current behavior of serving user doc:

The bahaviour of the master branch

(after adding empty to enable the markdown:)

(no pop up, when mouse is over; no response when clicked)

yucheng11122017

Hi @yiwen101 thanks for the work! Some more corrections

packages/core/src/html/includePanelProcessor.ts

yucheng11122017 · 2024-03-14T14:10:10Z

packages/core/src/html/includePanelProcessor.ts

@@ -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;


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?

Co-authored-by: Chan Yu Cheng <[email protected]>

yucheng11122017

Hi @yiwen101 thanks so much for cleaning up the code! I think using cheerio makes it more consistent with the rest of the code base. Just addition of one comment and I think this PR is ready to merge.

Thanks for the good work!

yucheng11122017 · 2024-03-16T06:06:22Z

packages/core/src/html/includePanelProcessor.ts

+      if (footnodeHrefs.length > 0) {
+        const tempFootnotes = $('<mb-temp-footnotes></mb-temp-footnotes>');
+        footnodeHrefs.forEach((href) => {
+          const listItem = $('<li></li>').attr('id', href.substring(1))


Could you add a comment to explain why we need to do a substring here? It is just to get rid of the hash in front right?

Thank you for the comment; has added the the latest commit

packages/core/src/html/includePanelProcessor.ts

Co-authored-by: Chan Yu Cheng <[email protected]>

Debug cannot import footnote from hash

22b9289

yiwen101 added 2 commits March 4, 2024 04:19

Update includePanelProcessor.test.ts

8b3cc6d

Update includePanelProcessor.test.ts

c7d4915

kaixin-hc reviewed Mar 4, 2024

View reviewed changes

packages/core/test/unit/html/includePanelProcessor.test.ts Outdated Show resolved Hide resolved

packages/core/src/html/includePanelProcessor.ts Outdated Show resolved Hide resolved

Fix include footnote bug in user doc

168dd4c

yiwen101 and others added 3 commits March 4, 2024 12:25

Fix wrong spelling

f285220

Improve readability of test

eecebc2

Merge branch 'master' into include_footnote

a68e82a

kaixin-hc reviewed Mar 4, 2024

View reviewed changes

packages/core/test/unit/html/includePanelProcessor.test.ts Outdated Show resolved Hide resolved

yucheng11122017 reviewed Mar 4, 2024

View reviewed changes

packages/core/test/unit/html/includePanelProcessor.test.ts Outdated Show resolved Hide resolved

yucheng11122017 reviewed Mar 4, 2024

View reviewed changes

yiwen101 marked this pull request as draft March 5, 2024 03:41

yiwen101 added 6 commits March 14, 2024 15:00

Merge branch 'master' into include_footnote

52c316d

Fix bug

1f9dce4

Update test

6a441b3

Improve readability of the test

a65218a

Improve getFootnodeNumber

e77a7bc

Merge branch 'master' into include_footnote

bd50434

yiwen101 marked this pull request as ready for review March 14, 2024 09:12

yucheng11122017 reviewed Mar 14, 2024

View reviewed changes

yiwen101 and others added 3 commits March 14, 2024 22:45

Update packages/core/src/html/includePanelProcessor.ts

1e8a14e

Co-authored-by: Chan Yu Cheng <[email protected]>

Improve code quality

3146481

Update includePanelProcessor.ts

003eeb7

yiwen101 and others added 3 commits March 14, 2024 23:46

Update test

53e3147

Remove omitted debugging log

1b9f432

Merge branch 'master' into include_footnote

db02850

yucheng11122017 approved these changes Mar 16, 2024

View reviewed changes

yiwen101 added 2 commits March 17, 2024 15:36

Merge branch 'master' into include_footnote

9620126

Add comment

54cc49f

yucheng11122017 reviewed Mar 17, 2024

View reviewed changes

packages/core/src/html/includePanelProcessor.ts Outdated Show resolved Hide resolved

yiwen101 and others added 2 commits March 17, 2024 23:36

Update packages/core/src/html/includePanelProcessor.ts

72168e0

Co-authored-by: Chan Yu Cheng <[email protected]>

Merge branch 'master' into include_footnote

d555215

yucheng11122017 merged commit dd4bfd3 into MarkBind:master Mar 18, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Debug cannot import footnote from hash #2441

Debug cannot import footnote from hash #2441

yiwen101 commented Mar 3, 2024 •

edited by yucheng11122017

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

kaixin-hc left a comment

yiwen101 commented Mar 4, 2024 •

edited

Loading

kaixin-hc commented Mar 4, 2024

yucheng11122017 left a comment •

edited

Loading

yucheng11122017 Mar 4, 2024

yiwen101 Mar 14, 2024

yiwen101 Mar 14, 2024 •

edited

Loading

yucheng11122017 Mar 4, 2024

yiwen101 Mar 14, 2024

yucheng11122017 Mar 14, 2024

yiwen101 Mar 14, 2024

yiwen101 Mar 14, 2024

yiwen101 commented Mar 5, 2024

yiwen101 commented Mar 5, 2024

yiwen101 commented Mar 14, 2024 •

edited

Loading

yucheng11122017 left a comment

yucheng11122017 Mar 14, 2024

yucheng11122017 left a comment

yucheng11122017 Mar 16, 2024

yiwen101 Mar 17, 2024

Debug cannot import footnote from hash #2441

Debug cannot import footnote from hash #2441

Conversation

yiwen101 commented Mar 3, 2024 • edited by yucheng11122017 Loading

codecov bot commented Mar 3, 2024 • edited Loading

Codecov Report

kaixin-hc left a comment

Choose a reason for hiding this comment

yiwen101 commented Mar 4, 2024 • edited Loading

kaixin-hc commented Mar 4, 2024

yucheng11122017 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiwen101 Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiwen101 commented Mar 5, 2024

yiwen101 commented Mar 5, 2024

yiwen101 commented Mar 14, 2024 • edited Loading

yucheng11122017 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yucheng11122017 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yiwen101 commented Mar 3, 2024 •

edited by yucheng11122017

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

yiwen101 commented Mar 4, 2024 •

edited

Loading

yucheng11122017 left a comment •

edited

Loading

yiwen101 Mar 14, 2024 •

edited

Loading

yiwen101 commented Mar 14, 2024 •

edited

Loading