-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Always call evalscripts
in fill
.
#110
Conversation
(per suggestions on slack, I'm happy to move the tests into their own test specific to |
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.
Looks good, thanks :)
src/content/api.jl
Outdated
fade ? | ||
@js_(o, Blink.fill($sel, $html)) : | ||
@js_ o document.querySelector($sel).innerHTML = $html | ||
@js_(o, Blink.fill($sel, $html, $(fade ? 1 : 0))) |
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.
Is the fade ? 1 : 0
really necessary?
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.
Oops, no, it's definitely not. Thanks. I think I was worried that javascript doesn't have values for true
or false
, but that's silly. XD
Fixed in the rebased version i just pushed! :) thanks!
test/runtests.jl
Outdated
@@ -13,4 +13,37 @@ w = Window(Blink.@d(:show => false)); sleep(5.0) | |||
|
|||
@test sprint(Blink.jsexpr, :(Dict("a" => 1, :b => 10))) == "{\"a\":1,b:10}" | |||
|
|||
@testset "content! Tests" begin |
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.
Moving this testset to test/api.jl
would be great :)
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.
Oops! Thanks. I started rebasing this, but never finished it looks like.. Let me push that up now!
Includes a @test_throws which verifies the bug in JuliaGizmos#109. This passes before the fix: Test Summary: | Pass Total content! Tests | 7 7
7104f25
to
57fc28e
Compare
Moves the fade/no-fade decision from julia down into javascript, so that more code can be reused between the two. This also simplifies the `content!` function body, which will make fixing JuliaGizmos#108 a bit easier. Updates "Fade False" test to not @test_throws. Tests still pass.
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.
Cool, fixed those things you mentioned! :)
Thanks! |
Fixes #109.
Moves the fade/no-fade decision from Julia down into javascript, so that
more code can be reused whether or not
fade == true
.This also simplifies the
content!
function body, which will makefixing #108 a bit easier.