-
Notifications
You must be signed in to change notification settings - Fork 173
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
Experiment: add support for distinguishing hover & click events #1979
base: main
Are you sure you want to change the base?
Conversation
…tomEvent detail option
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.
I’m cool with this conceptually, but I’m wondering if we can
- create a custom subclass of Event instead of using CustomEvent
- expose the sticky state as event.sticky instead of event.detail
If a subclass isn’t possible, then I would prefer detail to be an object {sticky} instead of the boolean for better future compat.
I changed it to use an Event subclass, but jsdom is still unhappy. There are lots of errors like this:
The error is thrown from a generated file that is not in source control, but here are the relevant lines from my local copy. I am not familiar with jsdom, but some searching found a similar issue that was mitigated with a "fix" that looks similar to what Plot already does:
which sounds similar to (or the same as) what Plot already does: Line 23 in 48b71d7
I don't quite understand what is happening yet. Will have another look tomorrow. |
…, but alias the global jsdom EventTarget to the window EventTarget...
…op level, but alias the global jsdom EventTarget to the window EventTarget..." This reverts commit 73a0e67.
Moving the event definition inside the I thought it might help since by the time the function runs it's definitely inside the right global context, but I wonder why that wasn't the case outside the function. |
As I understand it it's because it happens earlier and thus the class extends whatever is the The addition of the custom Please add a representative use case in test/plots/ so we can test these changes more easily. (See https://github.com/observablehq/plot/blob/main/test/plots/pointer.ts#L28 for an example.) |
I'm not sure if it's due to this issue or something else but the test command fails to regenerate output files if I delete them as per the instructions for regenerating the snapshot tests. This is on an M2 mac on Sonoma 14.2.1 with node v21.5.0. I tried generating an initial snapshot manually, but this gets rejected due to differences between the true output and the manual version. Comparing the HTML I copied from a notebook and the HTML in the pointerViewof output test, there seem to be lots of small differences so it doesn't seem to trivial to make a correct snapshot manually. Maybe someone else can generate the initial snapshot? Let me know if the general idea of the test is reasonable – it dynamically shows the click selection in a textarea similar to the way the viewof test shows the value. |
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.
I'll take a pass at generating the test
To generate the test output I did:
I also had to run
And finally
I don't think I can push to your branch, though. |
Co-authored-by: Philippe Rivière <[email protected]>
Thank you! I did not know about |
In the brush PR #1653 I'm following a slightly different pattern (with the same goal of telling the listener whether we're "browsing" or "selecting"). This PR attaches a .done property to the value on brush Pros and cons:
Whatever route we take we should try to be as generic as possible (and as consistent as what makes sense). |
Another aspect of generality is whether you anticipate ever wanting to support simultaneous hover / click selections. With the current event stream we assume that once a new non-sticky event comes in, the selected datum has been deselected. I've found the ability to support hovers and clicks at the same time to be very useful in more app-like contexts, where they can be used to show comparisons between the selected element and other (hovered) elements. |
I think that the information about the kind of selection should be communicated out-of-band, ie. not on the datum itself. I think this means that it would either be a property of the event object, or a wrapper object that contains the datum along with any metadata, eg. To me, the word There's also a third type of interaction one can get with some styluses, which is a true "hover" interaction where the cursor does not even touch the tablet: https://astropad.com/blog/apple-pencil-hover-everything-you-need-to-know/. I think there's a way to tell these apart from "touch" pointer events using the PointerEvents API but I'm not sure. |
We won't change plot.value, but it's worth thinking about exposing this alongside the value; I agree that done is a very limiting word. The example use case (click to fix 1 point & hover to compare) is very compelling. Reminds me of the sleepwalk UI to read distances on a (UMAP-learn) map: https://anders-biostat.github.io/sleepwalk/ & https://observablehq.com/@fil/umap-sleepwalk |
(and run prettier)
I noticed that on my iPhone, the example notebook only ever sets the |
Re: CI failure: looks like there was a reason we moved the event inside the |
This is a tiny experimental patch that addresses #1832 by sending Boolean "sticky" information via the event's
detail
option.Plot's behavior is otherwise unchanged. Since the sticky data is part of the event, a user can now create a generator cell that responds to the "substream" of click-relevant events. Example:
There might be unfavorable trade-offs to the idea of using the
detail
option for this, and thenull
handling is somewhat untidy (though maybe there's a better way?) so please feel free to close if it isn't a good idea. But I was surprised and pleased with the fact that this was such a small code change.Edit: lol, it fails tests. Sorry! I forgot to check that... Good thing I labeled this an experiment. The issue is that the event is now a
CustomEvent
, since that's the type that supports addingdetail
.