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 preamble and copy assets #47

Merged
merged 6 commits into from
Jul 8, 2021
Merged

Conversation

start974
Copy link
Contributor

I have added preamble to use package style "alectryon.sty" and "tango_subtle.sty".
And I add a copy of their assets styles files associate to destination.

I cannot compile the PDF associate, for this latex. I mean, we need to remove docutils packages imported, but I can't found this options in documentation.

@@ -38,8 +38,10 @@ class ASSETS:

ALECTRYON_CSS = ("alectryon.css",)
ALECTRYON_JS = ("alectryon.js",)
ALECTRYON_STY = ("alectryon.sty",)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be in latex.py rather than in html.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a assets.py to group all ASSET class.
And move function copy_assets.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to keep html.py and latex.py self-contained, keeping the assets in each of the relevant classes. WDYT?

Copy link
Contributor Author

@start974 start974 Jun 30, 2021

Choose a reason for hiding this comment

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

I'm thinking that the copy_asset function doesn't really have a place in html.py anymore, since it's now also used to copy latex assets.
And I think is more elegant to have one class ASSET and make inheritance to make other assets class (even if is for one field).

But if you think it's better to just move the latex assets, I can roll back in this way.

Copy link
Owner

Choose a reason for hiding this comment

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

How about moving copy_asset to cli.py? It's only used there AFAICT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we can do this.
Because the function is use just in cli.py, and we set ASSETS.PATH in cli.py also (and change name to ASSETS_PATHS).

Copy link
Owner

Choose a reason for hiding this comment

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

Yup, I think that's perfect, thanks!

@cpitclaudel
Copy link
Owner

Great work, thanks! To modify the header, I think you want to remove or change things self.head_parts. This is the default definition, and in fact each string is the name of an attribute (self.head_prefix, self.requirements, etc.); so, alternatively, you could modify head_prefix etc. as needed.

    head_parts = ('head_prefix', 'requirements', 'latex_preamble',
                  'stylesheet', 'fallbacks', 'pdfsetup', 'titledata')

@start974 start974 changed the base branch from rst+latex to master July 2, 2021 09:38
@start974 start974 changed the base branch from master to rst+latex July 2, 2021 09:39

ADDITIONAL_HEADS = [
'<meta name="viewport" content="width=device-width, initial-scale=1">'
]

class ASSETS:
PATH = path.join(path.dirname(_SELF_PATH), "assets")

PATH = Path(__file__).parent / "assets"
Copy link
Owner

Choose a reason for hiding this comment

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

One problem with using Path classes everywhere is that other code doesn't always expect them, so in testing the previous patch I reverted some of these because they confused Sphinx.

Copy link
Contributor Author

@start974 start974 Jul 2, 2021

Choose a reason for hiding this comment

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

To fix it, I transform Path to str in sphinx.py

Copy link
Owner

Choose a reason for hiding this comment

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

That works, but it may also break other Alectryon drivers out there; there may not be too many, but I know I have at least one like that locally, so I'd prefer to stick with the string-based version, just to minimize changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I change it

Copy link
Owner

@cpitclaudel cpitclaudel Jul 2, 2021

Choose a reason for hiding this comment

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

Thanks, let me know when I should review/merge

@start974 start974 force-pushed the rst+latex branch 4 times, most recently from 245ef8f to 5872352 Compare July 2, 2021 12:02
@start974 start974 changed the base branch from rst+latex to master July 2, 2021 12:16
@cpitclaudel cpitclaudel merged commit a414830 into cpitclaudel:master Jul 8, 2021
@cpitclaudel
Copy link
Owner

Rebased and merged, thanks!

@cpitclaudel
Copy link
Owner

Released in v1.2

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