-
Notifications
You must be signed in to change notification settings - Fork 178
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
Upgrade pandoc to 2.14 and fix plugin breaks #427
Conversation
new citeproc filter name upgrade pandoc-xnos packages refs tomduck/pandoc-fignos#85 upgrade panflute to 2.0.5 includes fix to #386 (comment): Element "MetaList" received "CSL_Item" but expected <class 'panflute.base.MetaValue'> pandoc html: disable document-css in template From pandoc 2.11 (2020-10-11) changelog: > Add CSS to default HTML template (#6601, Mauro Bieg). This greatly improves the default typography in pandoc’s HTML output. The CSS is sensitive to a number of variables (e.g. mainfont, fontsize, linestretch): see the manual for details. To restore the earlier, more spartan output, you can disable this with -M document-css=false. pandoc 2.11.3 https://github.com/jgm/pandoc/releases/tag/2.11.3
…stock into dhimmel-pandoc-2.13
AppVeyor build 1.0.244 for commit b5233d9 is now complete. Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
@@ -10,7 +10,8 @@ | |||
<script type="module"> | |||
// which types of elements to add anchors next to, in "document.querySelector" | |||
// format | |||
const typesQuery = 'h1, h2, h3, [id^="fig:"], [id^="tbl:"], [id^="eq:"]'; | |||
const typesQuery = | |||
'h1, h2, h3, div[id^="fig:"], div[id^="tbl:"], span[id^="eq:"]'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is div[id^="fig:"]
the workaround for double table ids (on the div and table) mentioned at #425 (comment)? It works by only matching div ids that start with tbl:
as opposed to all elements?
Just to confirm, it is okay for us to proceed despite the duplicate ids, because this PR handles it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened tomduck/pandoc-tablenos#31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is div[id^="fig:"]the workaround for double table ids (on the div and table) mentioned at #425 (comment)? It works by only matching div ids that start with tbl: as opposed to all elements?
That's correct. I've essentially made the queries more specific. As long as pandoc/table-nos is consistent and never outputs a table without the div wrapper, this should be fine.
Looked at the issue, 👍 FWIW id's being unique isn't just a suggestion, it's a requirement of the spec and can cause unexpected behavior. It shouldn't cause anything to crash though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
As long as pandoc/table-nos is consistent and never outputs a table without the div wrapper, this should be fine.
Based on the early discussion in the issue, it seems like the most likely fix is that pandoc-tablenos stops adding the div wrapper for tables since the id is now set in <table>
.
So if we upgrade pandoc-tablenos, we might have to tweak this, but that's okay. Let's wait to see what solution pandoc-tablenos adopts and merge this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh if that’s the case we’ll need to re-add the plug-in to wrap tables in divs for scrolling purposes 😞
build/themes/default.html
Outdated
.csl-entry { | ||
margin-top: 20px; | ||
margin-bottom: 20px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduced to 15px. I wouldn't recommend going lower than that, as it becomes difficult to distinguish for sighted and hard-of-seeing people alike.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing https://ci.appveyor.com/api/buildjobs/992931201tet4bko/artifacts/manuscript-1.0.244-b5233d9.html and everything looks good (one small comment about references spacing).
Thanks a lot!
AppVeyor build 1.0.245 for commit ea9a48f is now complete. Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Will wait for @agitter to review before merging
@agitter following up in case you'd like to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I have two minor suggestions, both optional.
Co-authored-by: Anthony Gitter <[email protected]>
AppVeyor build 1.0.246 |
The builds are failing because adding panflute to the conda-installed packages revealed an actual conflict. panflute 2.1 supports pandoc versions 2.11.0.4—2.13.x. When we install panflute from pip, it can't check the pandoc version. The conda-forge recipe does check the pandoc version. Our environment appears to work based on the HTML artifacts above when we ran with panflute 2.1.0 and pandoc 2.14.0.1. Should we revert 9ae8d19 and proceed to merge? Edit: I asked about any known compatibility issues in sergiocorreia/panflute#192 |
This reverts commit 9ae8d19.
AppVeyor build 1.0.247 Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscingcontent/02.delete-me.md:44:aliqua content/02.delete-me.md:44:amet content/02.delete-me.md:44:consectetur content/02.delete-me.md:44:dolore content/02.delete-me.md:44:eiusmod content/02.delete-me.md:44:elit content/02.delete-me.md:44:incididunt content/02.delete-me.md:44:ipsum content/02.delete-me.md:44:labore content/02.delete-me.md:44:Lorem content/02.delete-me.md:44:magna content/02... |
Thanks all for the help! Especially @vincerubinetti for the frontend edits. The diff looks simple in the end, but this includes some major upgrades in our dependencies. |
* setup.bash: interactive script to guide setup merges manubot/rootstock#417 closes manubot/rootstock#401 * Add "gh repo create" to SETUP.md merges manubot/rootstock#419 closes manubot/rootstock#418 Co-authored-by: Daniel Himmelstein <[email protected]> Co-authored-by: Anthony Gitter <[email protected]> * BUILD_LATEX for basic LaTeX manuscript merges manubot/rootstock#384 refs manubot/rootstock#249 * Pandoc 2.14: update HTML plugins, CSL style, citekey syntax merges manubot/rootstock#427 Co-authored-by: Daniel Himmelstein <[email protected]> Co-authored-by: Anthony Gitter <[email protected]> Co-authored-by: nfry321 <[email protected]> Co-authored-by: Tiago Lubiana <[email protected]> Co-authored-by: Daniel Himmelstein <[email protected]> Co-authored-by: Anthony Gitter <[email protected]> Co-authored-by: Vincent Rubinetti <[email protected]>
merges manubot/rootstock#427 Co-authored-by: Daniel Himmelstein <[email protected]> Co-authored-by: Anthony Gitter <[email protected]>
supersedes #425