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

Add Model.js_on_change method #5498

Merged
merged 1 commit into from Dec 5, 2016
Merged

Add Model.js_on_change method #5498

merged 1 commit into from Dec 5, 2016

Conversation

@bryevdv
Copy link
Member

@bryevdv bryevdv commented Nov 30, 2016

  • issues: fixes #5435
  • tests added / passed
  • release document entry (if new feature or API change)

Allows to add a CustomJS callback to any backbone event. As a convenience, if the event name is the same as one of the model properties, the event "change:prop_name" is generated instead.

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Nov 30, 2016

ping @mattpap @birdsarah just want to get a quick look before I commit to writing the tests and docs for this. I opted for a new method .js_on_change instead of overloading the existing .on_change. I think it is better for users to be more explicit in this way and it is definitely cleaner to not pollute the python-centric CallbackManager. Additionally, to support events on streaming and patching, etc., the interface actually accepts any arbitrary Backbone event, not just property names (tho property names are converted to the appropriate "change" event as a convenience)

@bryevdv bryevdv force-pushed the bryanv/5435_generic_js_events branch 4 times, most recently from 62bd8a3 to 2bc6b21 Dec 1, 2016
@mattpap
Copy link
Contributor

@mattpap mattpap commented Dec 1, 2016

Two things shouldn't be forgotten is supporting flexx (on Python-side) and (native) functions for TS API.

super(options)
for attr, callbacks of @js_callbacks
for cb in callbacks
@listenTo(@, "#{attr}", () -> cb.execute(@))
Copy link
Contributor

@mattpap mattpap Dec 1, 2016

String interpolation is unnecessary here.

Copy link
Member Author

@bryevdv bryevdv Dec 1, 2016

yup thanks, it was changed from something I had earlier

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Dec 1, 2016

Two things shouldn't be forgotten is supporting flexx (on Python-side) and (native) functions for TS API.

The flexx comes along for the ride with CustomJS. I'll probably need help for a TS API

@bryevdv bryevdv force-pushed the bryanv/5435_generic_js_events branch from 2bc6b21 to 15877ee Dec 1, 2016
Allows to add a CustomJS callback to any backbone event. As a
convenience, if the event name is the same as one of the model
properties, the event "change:prop_name" is generated instead.
@bryevdv bryevdv force-pushed the bryanv/5435_generic_js_events branch from 15877ee to 57d9049 Dec 2, 2016
@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Dec 2, 2016

@mattpap @birdsarah this is ready for review if you have the chance

@mattpap regarding adding TS API in this case I think it's better to just let people add event handlers in the standard JS ways, e.g. listenTo, which is already possible, rather than adding a duplicate of the python API in JS just because.

@@ -95,5 +93,7 @@ def update(t):

source.stream(new_data, 300)

curdoc().add_root(column(row(mean, stddev, mavg), gridplot([[p], [p2]], toolbar_location="left", plot_width=1000)))

Copy link
Member Author

@bryevdv bryevdv Dec 2, 2016

I think it's better to demonstrate curdoc().add_root as late as possible, as a best practice. The resolution of model reference happens at add_root not at serialization, so confusing situations can happen if add_root happens before a model has changes that affect its references.

@@ -9,7 +9,7 @@

source = ColumnDataSource(data=dict(x=x, y=y))

plot = Figure(plot_width=400, plot_height=400)
plot = figure(plot_width=400, plot_height=400)
Copy link
Member Author

@bryevdv bryevdv Dec 2, 2016

noticed a couple of lingering uses of Figure, fixed these up in this PR

spy.reset()
expect(spy.called).to.be.false
m.baz = 10
expect(spy.callCount).to.be.equal 0
Copy link
Member Author

@bryevdv bryevdv Dec 2, 2016

If there's a way to make this test better I am all ears. spy seems to wrap the prototype or something, precluding per-instance resolution of methods. So this test just counts the total calls and makes sure that they match the expected counts for each attr.

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Dec 3, 2016

I'll merge this tomorrow absent objection

@jlstevens
Copy link
Contributor

@jlstevens jlstevens commented Dec 5, 2016

This looks great to me and I am looking forward to seeing this merged!

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Dec 5, 2016

@mattpap I didn't get around to merging this weekend, and I wanted to give you more time to comment too. I will merge this afternoon unless you voice any objection.

@bryevdv bryevdv merged commit 3ab765c into master Dec 5, 2016
1 check passed
@bryevdv bryevdv deleted the bryanv/5435_generic_js_events branch Dec 5, 2016
>>> plot.select(tags=['foo', 10])
[GlyphRenderer(id='1de4c3df-a83d-480a-899b-fb263d3d5dd9', ...)]
Or simply a convenient way to attach any necessary metadata to a model
Copy link
Member

@birdsarah birdsarah Dec 5, 2016

Thanks for doing this @bryevdv

tommycarstensen
Copy link

tommycarstensen commented on 57d9049 Mar 23, 2017

This sentence seems to never have been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants