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

Upgrade pandoc to 2.14 and fix plugin breaks #427

Merged
merged 11 commits into from
Jun 10, 2021
Merged

Upgrade pandoc to 2.14 and fix plugin breaks #427

merged 11 commits into from
Jun 10, 2021

Conversation

vincerubinetti
Copy link
Collaborator

@vincerubinetti vincerubinetti commented Jun 2, 2021

supersedes #425

dhimmel and others added 7 commits May 29, 2021 11:43
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
@AppVeyorBot
Copy link

AppVeyor build 1.0.244 for commit b5233d9 is now complete.

Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/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...
The rendered manuscript from this build is temporarily available for download at:

@@ -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:"]';
Copy link
Member

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

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.

Copy link
Member

@dhimmel dhimmel Jun 3, 2021

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.

Copy link
Collaborator Author

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 😞

Comment on lines 501 to 504
.csl-entry {
margin-top: 20px;
margin-bottom: 20px;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd be in favor of a slightly smaller margin between references than:

image

I think we do want some, but since reference sections can get quite long, perhaps we could get by with slightly less? Maybe 15 (or 50% to 75% of the current spacing)?

Copy link
Collaborator Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Here's 15. Looks good:

image

Copy link
Member

@dhimmel dhimmel left a 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!

@dhimmel dhimmel changed the title Upgrade pandoc to 2.13 and fix plugin breaks Upgrade pandoc to 2.14 and fix plugin breaks Jun 3, 2021
@AppVeyorBot
Copy link

AppVeyor build 1.0.245 for commit ea9a48f is now complete.

Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/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...
The rendered manuscript from this build is temporarily available for download at:

Copy link
Member

@dhimmel dhimmel left a 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

@dhimmel
Copy link
Member

dhimmel commented Jun 9, 2021

@agitter following up in case you'd like to review

Copy link
Member

@agitter agitter left a 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.

USAGE.md Outdated Show resolved Hide resolved
build/environment.yml Show resolved Hide resolved
dhimmel and others added 2 commits June 10, 2021 09:08
Co-authored-by: Anthony Gitter <agitter@users.noreply.github.com>
@AppVeyorBot
Copy link

AppVeyor build 1.0.246

@agitter
Copy link
Member

agitter commented Jun 10, 2021

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

@AppVeyorBot
Copy link

AppVeyor build 1.0.247

Found 52 potential spelling error(s). Preview:content/02.delete-me.md:44:adipiscing
content/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...
for commit eb824ce is now complete. The rendered manuscript from this build is temporarily available for download at:

@dhimmel dhimmel merged commit defe787 into manubot:main Jun 10, 2021
@dhimmel
Copy link
Member

dhimmel commented Jun 10, 2021

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.

trangdata added a commit to greenelab/iscb-diversity-manuscript that referenced this pull request Aug 5, 2021
* 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 <daniel.himmelstein@gmail.com>
Co-authored-by: Anthony Gitter <agitter@users.noreply.github.com>

* 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 <daniel.himmelstein@gmail.com>
Co-authored-by: Anthony Gitter <agitter@users.noreply.github.com>

Co-authored-by: nfry321 <58332182+nfry321@users.noreply.github.com>
Co-authored-by: Tiago Lubiana <tiago.lubiana.alves@usp.br>
Co-authored-by: Daniel Himmelstein <daniel.himmelstein@gmail.com>
Co-authored-by: Anthony Gitter <agitter@users.noreply.github.com>
Co-authored-by: Vincent Rubinetti <vince.rubinetti@gmail.com>
ploegieku added a commit to ploegieku/2023-functional-homology-paper that referenced this pull request Aug 6, 2024
merges manubot/rootstock#427

Co-authored-by: Daniel Himmelstein <daniel.himmelstein@gmail.com>
Co-authored-by: Anthony Gitter <agitter@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants