-
Notifications
You must be signed in to change notification settings - Fork 31
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
base: master
Are you sure you want to change the base?
Conversation
Update from original
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.
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.
🚢 🇮🇹 !! 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") "" |
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.
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
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.
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 |
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.
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 😜
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.
Oh I though there was the place where modals belong, maybe my mistake.
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.
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
src/common/HaskellDo/Toolbar/View.hs
Outdated
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) |
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.
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
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.
We're not using Bootstrap @javiertoledo , we're using Materialize 😄
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 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
Just remembered this PR, Looks like margins and syntax highlighting is not exported properly, maybe it is possible to fix it? |
Using
Regarding to syntax highlight, you might need to check if the current syntax highlighter CSS is restricted to There are also some CSS tricks that might be useful In Rails, there are some gems to use |
No description provided.