-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Adds mermaid.js
and highlight.js
for code formatting along with Fire
managed args to mkposter
#4
base: main
Are you sure you want to change the base?
Conversation
Adds `mermaid` and github theme `highlight.js` for code formatting along with `Fire` managed args to mkposter
flake8 docstring fix
use `highlightAll()`
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.
This looks great.
mkposters/mkposter.py
Outdated
@@ -42,8 +70,15 @@ def mkposter(datadir): | |||
html_out = rf"""<!doctype html> | |||
<html> | |||
<head> | |||
<body style="background-color:{background_color}"> |
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.
This isn't well-formed HTML: you've added an open body tag in the middle of the head.
Personally I subscribe to the "do all styling through CSS" approach, and to never use inline styles.
Perhaps the best thing to do here would be to allow for user-specified SCSS (alongside the markdown). And then let the background color just be set there if desired.
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.
Good point on user specified SCSS. I started editing the custom.scss
first and wanted to generalize at minimum the background color so added some non-ideal html. I agree adding a custom option for a user file is the right approach. I can work on that in this PR.
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.
Addressed in most recent push as scss
optional user specified path.
mkposters/mkposter.py
Outdated
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Roboto:300,400,400i,700%7CRoboto+Mono&display=fallback"> | ||
<link rel="stylesheet" type="text/css" href="style.css"/> | ||
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.5.1/styles/{code_style}.min.css"> | ||
<script src="https://cdnjs.cloudflare.com/ajax/libs/highlight.js/11.5.1/highlight.min.js"></script> | ||
<script>hljs.highlightAll();</script> |
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.
Can these two lines be optional (based on a particular magic value of code_style
) and bump the minor version number (as this is backward-incompatible).
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.
Sure good point. On some browsers the superfences just wasn't working so added the highlight.js
which seemed to do the trick.
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.
To future proof a bit more, what are your thoughts on working towards something like a user specified mkposters.yml
to mirror mkdocs.yml
design? That way we can add additional markdown extensions, and user specified scss. That said, it would significantly expand the scope of work, which I likely won't have bandwidth for, and may not be entirely useful if the target audience is researchers who just want easily create one page posters with pure markdown without caring about managing yaml and any other settings outside of markdown. I wonder if you have a vision for the API/goals of the project to help guide the development decisions for some of these types of custom additions?
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.
Right, different browsers have pretty different behaviour. There's a reason I mandated specifically Firefox in the README, as nothing else seemed to work reliably.
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.
As for having a mkposters.yaml
. I'd definitely be open to this, but likewise don't plan to implement this myself. MkPosters is just an afternoon project I created to avoid any more wrestling with LaTeX when making my own academic posters, and the default style is just the one I personally use for all my posters.
So MkPosters isn't a project I plan to offer "official" support for (unlike some of my other work), and I don't have any real vision/goals for this project. But if this is a tool that's useful to other people then I'm very happy to continue accepting any PRs from folks who find it useful enough to tweak it to their needs.
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.
Great, that was my impression as well. I honestly would prefer to avoid starting down the yaml path and would rather keep it as simple as possible from a user experience perspective. I like the idea of an optional user specified scss, which I can bundle in this PR.
Thanks again for sharing this work - I've demoed an internal project using the html rendered with mkposters and find it useful for quickly creating a poster style presentation given my rusty html. I've also shared the torchsde
-> diffrax
work in a couple internal Lunch 'n Learns which, along with your thesis, are awesome.
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.
Made code style and mermaid optional for backwards compatibility in latest commit.
…, mermaid support, and highlight.js code style
Sb mermaid
def mkposter( | ||
datadir: str, | ||
code_style: Optional[str] = None, | ||
mermaid: Optional[bool] = False, |
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.
I don't think is Optional
, just bool
.
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.
The goal with the optional type and default of None was to preserve backwards compatibility. Or do you suggest we move forward with these as required args, with no default values, and then bump the minor version?
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.
Sorry, I'm talking about the mermaid
argument here, not the code_style
argument. I just mean the existing behaviour is already mermaid=False
, and passing mermaid=None
does nothing.
code_style: Optional[str] = None, | ||
mermaid: Optional[bool] = False, | ||
scss: Optional[Union[str, pathlib.Path]] = None, | ||
port: Optional[int] = 8000, |
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.
Likewise not Optional
, just int
.
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.
(Likewise passing port=None
here isn't meaningful.)
mkposters/mkposter.py
Outdated
@@ -68,10 +121,14 @@ def mkposter(datadir): | |||
if not (_here / "third_party" / "dart-sass" / "SASSBUILT.txt").exists(): | |||
post_install(package_dir=str(_here)) | |||
|
|||
style_sheet = f"{_here}/style.scss" | |||
if scss: | |||
style_sheet = scss |
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.
I think it'd be better to look inside datadir
for style.scss
, just like how we currently look for poster.md
.
Do you think it'd be better to have custom styling completely override the default styling (as is the case at the moment) or just override 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.
Good point - we can look for style.css in datadir
. That also requires all referenced imports to be in the datadir. I was thinking if the user specifies a style sheet it would enable them to have a new custom.scss
and provide flexibility to change import logic that may or may not use the third_party
icons and stylesheets.
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.
Worth noting that the current implementation will just override/ignore any existing user-specified file.
I think the best overall behaviour here would be to check if the user has a style.scss
file inside datadir
, and to have an additional override_scss
flag passed to mkposter
, defaulting to False
. If override_scss==False
then the user-specified style.scss
is appended to the default style.scss
prior to running dart-sass. If override_scss=True
then the user-specified style.scss
is used instead of the default style.scss
when running dart-sass.
addressing comments Co-authored-by: Patrick Kidger <[email protected]>
…ir`, similar to how `poster.md` is looked for.
Adds:
mermaid.js
diagram support for rendering mermaid blockshighlight.js
, default github themeFire
background_color
str argument with default#FFFFFF
code_style
str arg forhighlight.js
themes with default 'github'port
int arg to specifyhttp.server
port, with 8000 as default.Example: