-
Notifications
You must be signed in to change notification settings - Fork 50
PureScript rewrite #90
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
Conversation
This is now ready for review. I might refactor the jQuery code a bit, but I'll probably leave that for a separate PR, since everything is working here. |
I'll merge this soon if there are no objections. I've tested it pretty thoroughly. @soupi What do you think? |
I'll take a good look at this this evening :) |
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 we can push this and refactor as we go. It looks a bit intimidating at first but I don't think it's hard to get into and make changes which is the important thing.
src/Main.purs
Outdated
code <- fold <$> (JQuery.select "#code_textarea" >>= JQuery.getValueMaybe) | ||
|
||
let displayPlainText s = | ||
JQuery.select "#column2" >>= \jq -> do |
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.
why not use do notation?
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, thanks.
src/Main.purs
Outdated
execute :: forall eff. JS -> JS -> BackendConfig -> Eff (dom :: DOM | eff) Unit | ||
execute js bundle bc@(BackendConfig backend) = do | ||
let html = joinWith "\n" | ||
[ "<!DOCTYPE html>" |
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 could be a bit more readable and easier to change with """
instead
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.
Done, thanks.
src/Main.purs
Outdated
cacheCurrentCode | ||
hideMenus | ||
|
||
getBackend :: Backend -> BackendConfig |
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 should go in Types.purs
imo.
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 moved all of this under API actually. To do what I wanted, it had to be that way to avoid a dependency cycle.
Thanks for the review @soupi, I'll get back to this today hopefully. |
I've refactored the view code a little bit. I think it's slightly nicer now, but we could still do some more work on it. |
Congrats 🎉 |
Closes #62.
I started rewriting things in small steps, as an exercise, instead of rewriting everything at once. This way, we'll at least start getting some benefit from the type system.
More to come...