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 event.target.value in Generators.input #1808

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Nov 11, 2024

Currently when you wrap an interactive element in a div, maybe for aesthetic reasons, or because you want to add a title, use the resize helper, etc., you can't use Generators.input to read the value — or you need to copy the value somehow to the top-level element, as we do in Plot with the figure element.

This, even though the input event bubbles up, which makes Generators.input slightly inconsistent.

This change makes Generators.input read the value on the input event’s target, making the most of the input event.

```js echo
function scatterPlot(width) {
  return Plot.plot({
    width,
  marks: [Plot.dot(cars, { x: "power (hp)", y: "economy (mpg)", tip: true })]
})};
```

<div id=chart class="grid grid-cols-1">
  <div class="card">${resize((width) => scatterPlot(width))}</div>
</div>

```js echo
const v = Generators.input(document.getElementById("chart"));
```

<pre>${JSON.stringify(v, null, 2)}</pre>

Note: if there are several inputs inside the "chart", so that whichever input (chart) you're touching returns its value. To combine values, you still need to use Inputs.form (but maybe Inputs.form could benefit from the same treatment?)

(OP: #1806 (reply in thread))

Leaving as a draft as there may be unwanted consequences that I'm not seeing. Though I guess a developer should explicitly opt out of event bubbling if they don't want to expose the input up in the hierarchy?

@Fil Fil requested a review from mbostock November 11, 2024 11:07
@Fil Fil changed the title use event.target.value use event.target.value in Generators.input Nov 11, 2024
@Fil

This comment was marked as resolved.

@Fil Fil marked this pull request as ready for review November 13, 2024 08:30
Comment on lines 6 to +7
let value = valueof(element);
const inputted = () => change(valueof(element));
const inputted = (event) => change(valueof(event.target));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that the initial value is valueof(element) while subsequent values are valueof(event.target) suggests that this may not be desirable. I see the benefit of being able to listen to a parent element, but in that case, how to determine the initial value of the generator? The generator would need to search within the container for an element that exposes a value. (And pick the first of multiple, arbitrarily?)

We could have the resize helper proxy the value of the returned element. Then you could say:

```js
const chart = resize((width) => scatterPlot(width));
const value = Generators.input(chart);
```

<div class="grid grid-cols-1">
  <div class="card">${chart}</div>
</div>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good remark, I hadn't thought about the initial value. I think I'd prefer the “search” approach because it would be easier to use — and also because it fits my mental model of an event that bubbles up (wants to be seen and useful).

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.

2 participants