-
Notifications
You must be signed in to change notification settings - Fork 880
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
SolaraViz Updates #2299
SolaraViz Updates #2299
Conversation
Performance benchmarks:
|
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 great!
Not directly for this PR, but are you still supporting a name change (we were considering ModelExplorer
right)?
Which objects are allowed/not allowed? |
I didn't voice this concern in the past: but before this PR, the reset button was dangerously close to the play button, that people could accidentally fat-fingered it. But now it seems safer. |
One of the tests in test_solara_viz.py failed. |
Good question, I don't know. I would guess only strings work, but I will test this out |
In this case, it's fine since I haven't provided any change requests, so the agreement is implicit, but in the future, I'd appreciate if the visualization PR gets merged when I explicitly approve, since @EwoutH hadn't dev-ed in the viz area yet. |
Sounds fine to me! But yes, since you already replied without any change requests I thought you are fine with the changes. And appreciated the heads-up to the failing test |
Some more work on the SolaraViz Frontend. Highlights
make_text
function. It was used likemake_text(renderer)
, but now you can userenderer
directly as a component