Markdown footnotes not working
What does this MR do?
The sanitization filter removes all the footnote classes and ids. We now enable ids for footnotes, and add back the classes for wrapping the footnotes (allowing them to be styled properly).
Footnotes are numbered the same - the first one has id=fn1, the second is id=fn2, etc. In order to allow footnotes when rendering multiple markdown blocks on a page, we need to make each footnote reference unique.
We add a random number to each footnote (the same number can be used for a single render). So you get id=fn1-4335 and id=fn2-4335.
Note to UI/FE: In order to allow for easier styling, classes generated by CommonMark were carried through into the footnotes.
- The entire footnote section is wrapped in
<section class="footnotes"> - The back reference link has the
footnote-backrefclass - The superscripted reference has the
footnot-refclass
<p dir="auto">
one footnote<sup class="footnote-ref"><a href="#fn1-9351" id="fnref1-9351">1</a></sup>
</p>
<section class="footnotes">
<ol>
<li id="fn1-9351">
<p>one <a href="#fnref1-9351" class="footnote-backref"><gl-emoji title="leftwards arrow with hook" data-name="leftwards_arrow_with_hook" data-unicode-version="1.1">↩</gl-emoji></a></p>
</li>
</ol>
</section>What are the relevant issue numbers?
Closes #26375 (closed)
Does this MR meet the acceptance criteria?
-
Changelog entry added, if necessary -
Documentation created/updated -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer
Merge request reports
Activity
assigned to @digitalmoksha
added 2 commits
added 2 commits
added 2 commits
marked the checklist item Changelog entry added, if necessary as completed
marked the checklist item Tests added for this feature/bug as completed
marked the checklist item Conforms to the code review guidelines as completed
marked the checklist item Conforms to the merge request performance guidelines as completed
marked the checklist item Conforms to the style guides as completed
added 2 commits
added 2 commits
changed milestone to %11.8
added 182 commits
-
30fb63a0...1b3affaf - 180 commits from branch
master - fbccd8aa - Properly process footnotes in markdown
- b48854d3 - Update nokogiri to 1.10.0
-
30fb63a0...1b3affaf - 180 commits from branch
added 2 commits
@engwan If you have some time, would you mind reviewing this for me?
@annabeldunstone I wanted to call attention to the new classes I've added (detailed in the description). We could make them anything - these happen to be what CommonMark generates by default, and I don't anticipate any conflicts. But if they need changing, just let me know what they should be.
assigned to @engwan
-
@digitalmoksha Added a few questions / comments.
A minor concern is the backref link which is rendered using a
↩ 
It doesn't look good for me. What do you think @annabeldunstone? This could be handled in a separate issue though.
assigned to @digitalmoksha
I wanted to call attention to the new classes I've added (detailed in the description). We could make them anything - these happen to be what CommonMark generates by default, and I don't anticipate any conflicts. But if they need changing, just let me know what they should be.
@digitalmoksha The class names look good to me. I don't see any
footnoterelated CSS within GitLab so we should be good👍 A minor concern is the backref link which is rendered using a
↩ This is what we have currently and I think it's fine. Thanks for checking @engwan!
-
@engwan I made changes for the review comments, can you take a another quick look when you have a chance?
assigned to @engwan
-
@annabeldunstone thanks for taking a look
🙇 We might want to consider a little styling with the footnotes in the future. I can envision a thin line separator, maybe a slightly smaller font (like real footnotes). And I agree with @engwan , maybe different backref icon (or I would even suggest not using an icon, just the actual unicode return
↩character, which would be more sublte).But I think that could all be considered under a different issue/MR (I'll open one just to have it), certainly down the road.
assigned to @digitalmoksha
-
@digitalmoksha LGTM
👍 -
@godfat I was wondering if you would have time to review this as a maintainer? I think it's ready to go
🚀 assigned to @godfat
added banzai label
@digitalmoksha Thank you! Although I left a few comments and open discussion, this looks really great to me. I think we can do some of them in the next iteration, not really needed right here. What do you think?
assigned to @digitalmoksha
mentioned in issue #56272 (moved)
-
Created https://gitlab.com/gitlab-org/gitlab-ce/issues/56272 for styling the footnotes.
-
@godfat thanks for the review! I think I've addressed most of your comments. Can you take another look when you have a chance?
assigned to @godfat
@digitalmoksha Thank you! Looks great to me. Just a bit few minor stuffs and still need to update nokogiri for QA.
assigned to @digitalmoksha
mentioned in merge request gitaly!1044 (merged)
-
@godfat I think I've addressed everything. Can you take a final look?
assigned to @godfat
@digitalmoksha Thank you! Looks great. This is a bit behind master but I think it's unlikely to conflict with the others, and unlikely to conflict with EE, and unlikely to break QA so I'll just merge as.
mentioned in commit ae0b8880
added devopsplan label
