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

Use JSStrings and macros from JSExpr #125

Merged
merged 3 commits into from
Jul 22, 2018
Merged

Conversation

rdeits
Copy link
Collaborator

@rdeits rdeits commented May 30, 2018

Updated version of #89

This is a step towards JuliaGizmos/WebIO.jl#48 . I didn't introduce the trait yet, but it will be trivial to do once Blink depends on WebIO.

@shashi I made some guesses about the right way to do this. The tests pass, and the p5.js example from WebIO works, and MeshCat.jl works, but I'm still guessing I got at least something wrong here 😉

@shashi
Copy link
Member

shashi commented Jun 6, 2018

Appveyor says

   Evaluated: Any[140, 200] == [100, 200]

I get this test failure on my Fedora machine as well

  Expression: size(w) == [150, 100]
   Evaluated: Any[150, 101] == [150, 100]

@pfitzseb
Copy link
Member

pfitzseb commented Jun 6, 2018

Looks good to me.
The appveyor failure is unrelated (I'll look into that when I find the time). Tests pass locally on Linux -- I suspect the off-by-one error is some weird pixel measuring (and rounding) thing.

Do you think we should merge this now or do you want to keep working on this branch, @rdeits?

@rdeits
Copy link
Collaborator Author

rdeits commented Jun 6, 2018

I don't have any more changes planned. My only concern is that I wanted to make sure that my assumption that what was previously Blink.jsstring(x) is now more like string(JSExpr.jsstring(x)...) is correct. That is, jsstring now returns a collection which needs to be concatenated to make a single JS string. is that assumption right?

@rdeits
Copy link
Collaborator Author

rdeits commented Jul 21, 2018

bump?

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