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 export PDF functionality #114

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

juanjoman
Copy link
Member

No description provided.

Added the functionality to export PDFs to the ToolBar
Fixed the way Haskell-do reacts when wkhtmltopdf is not installed.
Changed the way Haskell-do detects that wkhtmltopdf is not in $PATH.
Copy link
Member

@javiertoledo javiertoledo left a comment

Choose a reason for hiding this comment

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

🚢 🇮🇹 !! That's a nice feature, just leave a few comments

@@ -134,7 +137,26 @@ update ToggleError state = do
localIO toggleError
return state

update ConvertToPDF state = do
checkIfInstalled <- atRemote . localIO $ readProcess "find" ["/usr/bin","-name","wkhtmltopdf"] []
(errorCode,_,_) <- atRemote . localIO $ readCreateProcessWithExitCode (shell "which wkhtmltopdf") ""
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you're checking the existence of the wkhtmltopdf binary twice here. I'd use which, that not only tells you if the file exists or not, but also what's its location. I guess there will be people out there that have installed wkhtmltopdf somewhere else, so for them, I think that this code will fail when it tries to find the binary in /usr/bin

Copy link
Member Author

@juanjoman juanjoman Aug 16, 2017

Choose a reason for hiding this comment

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

Hi @javiertoledo! Thats true, I think that would be enough with the which command, my mistake 👍

packageEditorModal -- Apparently, if we put this line
openProjectModal -- under this one. The open project modal doesn't work
modalPromptPlaceholder "newDirectoryModal" "New Directory" "Choose a name for the new directory"
convertToPDFModal
convertToPDFModalFail
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you're rendering the modals inside of the toolbar. If there's no reason for that, I'd suggest to render them in the body element instead, final result won't differ, but I think that makes more sense 😜

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I though there was the place where modals belong, maybe my mistake.

Copy link
Member

Choose a reason for hiding this comment

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

Actually it's a mistake by my part, instead of making a separate component for each modal I just jammed em all into the toolbar, which is awful, see #76

convertToPDFModalFail =
div ! id "convertToPDFModalFail" ! atr "class" "modal" $ do
div ! atr "class" "modal-content" $ do
h4 ("Error: wkhtmltopdf is not installed or a project has not been loaded" :: String)
Copy link
Member

Choose a reason for hiding this comment

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

Not crucial, but Bootstrap allow for much richer modals with buttons ant that stuff to make users feel like they have some kind of control over their machines, I think that a Dismiss button would be a nice addition.

BTW @NickSeagull, which version of Bootstrap are you using? Bootstrap 4 recently reached beta status, which obviously means that it's production-ready xDD

Copy link
Member

Choose a reason for hiding this comment

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

We're not using Bootstrap @javiertoledo , we're using Materialize 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to look for other Materialize components but I didn't found a better one for the messages 😮

Changed the way haskell-do checks if wkhtmltopdf is installed.
There is no need to keep the "readProcess" function.
Now It's in the same color as compilation errors
@NickSeagull
Copy link
Member

Just remembered this PR, Looks like margins and syntax highlighting is not exported properly, maybe it is possible to fix it?

@javiertoledo
Copy link
Member

javiertoledo commented Jun 5, 2018

Using wkhtmltopdf is always tricky, you might want to check the manual, there are a few options for setting document margins and other interesting details:

  ...
  -B, --margin-bottom <unitreal>      Set the page bottom margin
  -L, --margin-left <unitreal>        Set the page left margin (default 10mm)
  -R, --margin-right <unitreal>       Set the page right margin (default 10mm)
  -T, --margin-top <unitreal>         Set the page top margin
  ...

Regarding to syntax highlight, you might need to check if the current syntax highlighter CSS is restricted to @media screen, as that CSS is not rendered if you "print" the document.

There are also some CSS tricks that might be useful

In Rails, there are some gems to use wkhtml2pdf in a friendlier manner: wicked_pdf and pdfkit. If we end up doing the effort to get this right, it might be useful to have a wrapper library for haskell. There's even a C library that could be used to make a cleaner integration, I can't wait for the fun :-D

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.

3 participants