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 Better PDF plugin #142

Merged
merged 1 commit into from Feb 2, 2021
Merged

added Better PDF plugin #142

merged 1 commit into from Feb 2, 2021

Conversation

MSzturc
Copy link
Contributor

@MSzturc MSzturc commented Jan 31, 2021

No description provided.

@lishid
Copy link
Collaborator

@lishid lishid commented Feb 1, 2021

https://github.com/MSzturc/obsidian-better-pdf-plugin/blob/ea50255f5994f08a2f50ecb85e2075b7c3d53af0/main.ts#L126
Given you aren't unloading anything, you won't need to make a child to manage the lifecycle of this.

Just inline the entire PDF rendering code in your post processor should be fine.

One thing to make sure though:
Here you're calling a callback which splits the async function. You should instead use for (let node of nodes) to ensure your async function is continuous. Same here, make this an await.

@MSzturc
Copy link
Contributor Author

@MSzturc MSzturc commented Feb 1, 2021

Thank you for your constructive feedback. I tried to include all the comments in the code. Please look over it again, you may find further points that need to be improved.

@lishid
Copy link
Collaborator

@lishid lishid commented Feb 1, 2021

That's a good step yeah.

I was probably unclear in my last comment:
I think you should just remove the class PDFRenderNode and move everything from its onload() inside your post-processor, replacing this line.

@MSzturc
Copy link
Contributor Author

@MSzturc MSzturc commented Feb 1, 2021

I've removed PDFRenderNode class and refactored parameterReading into separate method. Please look over it again.

@lishid
Copy link
Collaborator

@lishid lishid commented Feb 2, 2021

Looks great! Let's get this merged. Seems like there's a conflict so you might have to rebase your PR or send it fresh again.

@MSzturc
Copy link
Contributor Author

@MSzturc MSzturc commented Feb 2, 2021

All conflicts are now resolved. Feel free to merge my PR

@lishid lishid merged commit 8437167 into obsidianmd:master Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants