-
Notifications
You must be signed in to change notification settings - Fork 50
Migrate to Halogen #215
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
Migrate to Halogen #215
Conversation
It looks like the CORS issue is introduced in #212. For testing, I'm just pointing to:
Here's a snapshot of a branch with the last working commit before CORS issues. You can cherry-pick the next commit to trigger the failure. Best guess on a possible cause (comment). |
I followed up on that comment and that's absolutely the issue. I don't know why using a |
I've updated the request in question and things appear to be working correctly now. Now that I'm able to run things properly I can test this out and make sure it's ready for review. |
@@ -181,6 +181,7 @@ body { | |||
left: 0; | |||
right: 0; | |||
bottom: 0; | |||
font-size: 13px; |
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.
Setting the font size on the editor container works fine; setting it via the Ace bindings is not currently working and I'll need to file a bug with purescript-contrib/purescript-ace
.
If we agree to move this direction and this PR looks good, then I'd like to explore deploying a new version of Try PureScript for 0.14 before we start adding any new features. |
I suspect that you were sending a JSON-encoded string to the server, e.g. the request might have looked like
as opposed to what the server was expecting, which would have been
This is where the "network" tab in the browser dev tools comes in handy. |
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'll leave it up to you to decide what you want to do about the debouncing. This looks good - I'm not a Halogen expert but the code is still relatively easy to follow, so nice work 👍
This PR continues #199 by replacing the existing jQuery with Halogen, except for the iframe -- the iframe deserves its own conversation and so I've left it untouched for this pull request. I have avoided changing any features and the application should look and behave the exact same as it currently does.
Well...there's one difference: the old code tried to apply the Ace 'Dawn' theme, but it wasn't working; in this version the dawn theme is present. I can remove the theme altogether if desired, which I believe will default to the TextMate theme.
To review the code I would recommend pulling up the old
index.html
and oldMain.purs
files against the newContainer.purs
Halogen component, as the component wraps in functionality that was previously spread across those two files. To review the functionality I'd just make sure the editor works fine for you. I tested it by putting up the old editor and the new one side-by-side in browser tabs.I've avoided doing some things that could be changed in the future if we want to clean things up a little:
My hope is that this unblocks some of the useful improvements @milesfrain provided a proof-of-concept for (like storing contents in the URL, re-enabling saving gists, and so on); with everything converted to Halogen those could be introduced one-by-one.
Here's a brief demonstration of the before-and-after. First, the current
try.purescript.org
:Old.mov
Next, the Halogen version on localhost:
New.mov