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

Added a caption for PlantUML diagrams #39

Merged
merged 45 commits into from
Apr 14, 2019
Merged

Conversation

SommerEngineering
Copy link
Contributor

Dear community,

I added a caption for PlantUML diagrams, as discussed at the mailing list, cf. https://groups.google.com/forum/#!msg/pandoc-discuss/xItcBa8OtuI/s9MoBfc_CwAJ

@SommerEngineering
Copy link
Contributor Author

I updated also the README to use the new captions. The Makefile is fine.

Copy link
Member

@tarleb tarleb left a comment

Choose a reason for hiding this comment

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

Cool!

plantuml/readme.md Outdated Show resolved Hide resolved
plantuml/plantuml.lua Outdated Show resolved Hide resolved
plantuml/readme.md Outdated Show resolved Hide resolved
@SommerEngineering
Copy link
Contributor Author

SommerEngineering commented Jan 7, 2019

Thanks @tarleb for your feedback. I will fix that as soon as possible.

@SommerEngineering SommerEngineering mentioned this pull request Jan 7, 2019
@tarleb
Copy link
Member

tarleb commented Jan 9, 2019

Clean and beautiful code, and well documented, too! There are currently no tests for the new filters, which is why the CI build fails. Can you add some?

I think the figure numbering filter deserves a separate pull request, so we don't mix different things in one PR.

@SommerEngineering
Copy link
Contributor Author

@tarleb I removed the figure numbering filter. I will create another PR for that. Tests are added.

@SommerEngineering
Copy link
Contributor Author

I created an issue for Pandoc: jgm/pandoc#5436

@tarleb
Copy link
Member

tarleb commented Apr 8, 2019

Thanks! I'll take a look soon.

@SommerEngineering
Copy link
Contributor Author

The Windows issue jgm/pandoc#5436 was fixed with Pandoc 2.7.2. I updated the output.html and added a hint regarding the usage of the Java variables.

@tarleb
Copy link
Member

tarleb commented Apr 13, 2019

I finally found the time to start a review. This is a great PR and probably already in mergeable state. I'll leave some more comments inline over the next day, but those should be understood less as "needs to be changed" but more as "what do you think about".

diagram_generator/README.md Outdated Show resolved Hide resolved

- Any LaTeX installation. This should be configured so that
missing packages are installed automatically. This filter uses the
`pdflatex` command which is available by the system's path. Alternatively,
Copy link
Member

Choose a reason for hiding this comment

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

In the future (i.e., once jgm/pandoc#5221 has been resolved), we could also use pandoc's --pdf-engine option to chose a LaTeX version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

diagram_generator/README.md Outdated Show resolved Hide resolved
diagram_generator/diagram_generator.lua Outdated Show resolved Hide resolved
diagram_generator/diagram_generator.lua Outdated Show resolved Hide resolved
diagram_generator/diagram_generator.lua Outdated Show resolved Hide resolved
diagram_generator/diagram_generator.lua Outdated Show resolved Hide resolved
diagram_generator/diagram_generator.lua Outdated Show resolved Hide resolved
diagram_generator/Makefile Outdated Show resolved Hide resolved
diagram_generator/README.md Outdated Show resolved Hide resolved
@tarleb
Copy link
Member

tarleb commented Apr 14, 2019

Two other small things: the filter name currently contains a underscore, while all other filters in this repo use a hyphen to separate words; it would be good to be consistent there. I think output.html should not have been part of the original plantuml filter, it can be removed.

Please let me know whether you prefer to do more work, or whether you'd rather see this merged right away. I'm okay with both options, we can still fix things after merging.

@SommerEngineering
Copy link
Contributor Author

Thank @tarleb for the advise :-) I fixed anything. The PR might be good to go now :-)

@SommerEngineering
Copy link
Contributor Author

By the way: PR #37 is might obsolete since Graphviz is included here.

@tarleb tarleb merged commit 1ed5cbf into pandoc:master Apr 14, 2019
@tarleb
Copy link
Member

tarleb commented Apr 14, 2019

Great, thanks!

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.

2 participants