Repository navigation
Conversation
Fixes koaning#42, fixes koaning#43. BarWidget's frontend now: - seeds its collections from the model's existing data at render time, instead of discarding constructor-provided data and immediately overwriting it with all-y_min bins (koaning#42) - registers change: handlers for data, n_bins, y_min, and y_max, so Python-side updates take effect without a re-render (koaning#42). The change:data handler skips the echo of its own model.set() in updateDataOut() (which fires change synchronously) to avoid a loop - scopes its collection-button queries to its own container instead of the whole document, so render no longer throws when the view isn't attached to the document yet, the initial .click() can't hit another widget's button, and multiple BarWidgets on a page no longer switch each other's active collection (koaning#43) width/height/collection_names changes still require a re-render. Tests: static regression checks on the bundle (same style as tests/test_scatter_widget_init_order.py), plus manual verification in a live Shiny for Python app (via shinywidgets): constructor seeding, Python-side data=/n_bins= pushes, and two BarWidgets coexisting without cross-contamination all work; no render-time console errors.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #42, fixes #43.
What was broken
#42 — BarWidget ignored Python and clobbered constructor data.
render()initialized every collection from scratch (new Array(n_bins).fill(y_min)) and immediately calledupdateDataOut(), which synced that zeroed array back to Python — discarding anydatapassed to the constructor. And with nomodel.on("change:...")handlers (unlike ScatterWidget, which has four), later Python-side updates todata/n_bins/y_min/y_maxhad no effect until a full re-render.#43 —
document-level button queries.render()ended withdocument.querySelector('button.control').click(), which throwsTypeError: Cannot read properties of null (reading 'click')in hosts that callrender()before attaching the view's element (e.g. Shiny for Python), and clicks another BarWidget's button when more than one is on the page. Thedocument.querySelectorAll('button.control')click handler similarly made every BarWidget's buttons switch every other BarWidget's active collection (in Jupyter too).The fix (
js/bar_widget.js, bundle rebuilt)seedFromModel()helper (re)fills each collection's bins frommodel.get("data"), falling back toy_minfor uncovered bins and clamping to[y_min, y_max]like drawn values. Called at render, so constructor data now survives — the initialupdateDataOut()just normalizes/round-trips it.change:handlers fordata(re-seed + redraw),n_bins(re-fit bins, keeping the ones that still exist), andy_min/y_max(redraw). Thechange:datahandler skips the echo ofupdateDataOut()'s ownmodel.set()(which fires change synchronously) via asyncingDataOutflag, so there's no update loop.document.querySelector(All)→container.querySelector(All), so render works before document attachment and widgets can't interfere with each other.width/height/collection_nameschanges still require a re-render — happy to add those here if you'd like.Testing
tests/test_bar_widget_python_sync.py: static regression checks on the bundle, same style as the existingtest_scatter_widget_init_order.py(all fail on the unpatched bundle).datapassed to its constructor #42/BarWidget queries document (not its own element) for control buttons: throws when the view isn't attached yet, and cross-contaminates multiple BarWidgets #43): constructor-seeded bars render, Python-sidewidget.data = ...andwidget.n_bins = ...update the chart in place, two BarWidgets on one page keep independent active collections, and there are no render-time console errors. All six checks fail on 0.5.2 and pass with this patch.