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

Add default file format when Quarto format is typst #7

Merged
merged 17 commits into from
Mar 20, 2024

Conversation

elipousson
Copy link
Collaborator

Happy to open a GitHub issue for discussion but, since this is such a small PR, I thought I'd go ahead and give this a try.

I also added the DarkFlagshipTerrastruct to the list of supported themes (it appeared to be missing but I can reverse that change if I missed something).

Also add DarkMauve and DarkFlagshipTerrastruct as explicit theme options
DarkMauve was already in the theme list
Also:

- Adds is_nonempty_string helper function
- Appends random number for diagram file name
- Use d2 file extension not txt for input
- Allow alternate syntax for attribute classes on code blocks
Copy link
Member

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Hey Eli!

Thanks for creating this PR.

Would you be able to add an entry to the CHANGELOG.md, describing each of the changes made in this PR?

In addition, I'm wondering why you add a random number to the filename, as this might lead to many duplicated files if the document is rerendered a couple of times.

Thanks!

@@ -121,13 +129,17 @@ local function render_graph(globalOptions)

-- Set default filename
if options.filename == nil then
options.filename = "diagram-" .. counter
options.filename = "diagram-" .. counter .. math.random (1, 1000)
Copy link
Member

Choose a reason for hiding this comment

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

Why add a random number to this filename?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had an issue with the extension not overwriting existing files that this seemed to resolve. I’ll try to put together a reprex (it may be specifically related to using knit_child?) to see if this random number assignment is a real fix or if there may be a better way to fix the issue.

@elipousson
Copy link
Collaborator Author

Thanks for the review!

I’ll go back to the file naming code and make sure to document the necessity of the change.

The change is detecting the d2 attribute class also introduce a small bug that I haven’t quite fixed yet.

I’ll get that sorted along with the CHANGELOG in the next week or two. Thanks for a cool extension!

Also:

- Add support for reading d2 diagrams from external files using `file` parameter. Block text is ignored if file parameter is supplied.
- Add support for alternate code block syntax without curly braces.
- Insert rendered diagrams into the Pandoc mediabag when `embed_type="link"`
- Refactor to add helper functions `setPreD2RenderOptions` and `setD2RenderFormat`
@elipousson
Copy link
Collaborator Author

I think I've almost got it right but I need to work out one more bug. I'll flag when it is ready for review again.

Also

- add animate_interval parameter
- drop setD2RenderFormat helper function
- use quarto.project.output_directory to set default folder
- revise check for d2 code block class
- replace codeblock text if file is provided
- add support for tala layout
@elipousson
Copy link
Collaborator Author

elipousson commented Mar 15, 2024

Dang, I think I've got some good improvements in this PR—support for external d2 files, more consistent handling of output folder paths, and those improvements for Typst output formats—but I'm 100% stuck on getting the raw embeds working again.

If you have a chance to take a look, @rcannood, I'd love to know what I'm doing wrong.

@elipousson
Copy link
Collaborator Author

Finally figured it out: I had an error in how I was writing the control flow in Lua but otherwise it all works as expected. Ready for a second review, I think!

Also:

- add check to see if file exists
- Improve handling of code block formatting (when echo=true)
- remove duplicative processing for options.pad and options.animate_interval
@rcannood
Copy link
Member

Happy to see you managed to find the issue! I intended to take a look at it, but didn't manage to find the time.

Could you merge the main branch into yours and move your examples to tests/? Currently the CI will only render the qmds, but it's better than nothing :)

@elipousson
Copy link
Collaborator Author

Added some basic examples to tests (along with a simple d2 file for testing).

I also added some partial support for code folding per the discussion on #6. Code folding will work in HTML output but there are some unresolved formatting issues with d2 code blocks (the gray background doesn't show in the HTML format).

@rcannood
Copy link
Member

LGTM!

Thanks @elipousson for making significant changes to the codebase!

Would you like to become a collaborator in this repository?

@rcannood rcannood merged commit 1f6fb19 into data-intuitive:main Mar 20, 2024
@elipousson
Copy link
Collaborator Author

@rcannood I'd be happy to! Time permitting, I'd love to figure out how to implement Quarto-style #| block options using https://github.com/coatless-quarto/codecelloptions if you're up for having it added. I also just spotted a typo (mt in the pandoc.insert.mediabag call instead of mimetype) which I can fix directly. I appreciate the invitation and the opportunity to contribute to such a handy extension.

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