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.
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
[Feature anywhere] Add annotation click handler #3777
[Feature anywhere] Add annotation click handler #3777
Changes from 12 commits
edab515
5fca51a
0fe8e82
325c9fc
09d6cbb
e4921c6
4cfdca4
5912441
76d8744
e469fe9
ed817b2
a53f8ab
618ef27
4669fca
67cc5f5
6d9b9b4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Should we have these actions split into 2? One for triggering the flyout opening, and another for triggering the custom tooltip? Curious of thoughts from @ashwin-pc on this as well.
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.
Depends on whether we want to keep events from the visualization generic and have the handler distinguish based off of the data. This way we will keep the events minimal and avoid adding specific info into the general framework.
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.
Yeah - I don't have a strong preference either way. Agreed we don't want to overload too specific of events here. Maybe one per
VisLayer
implementation makes sense - in this particular case, handling click/mouseover/mouseout actions for thePointInTimeEventsVisLayer
.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.
On second thought, perhaps we do want to have these split up. We will need a mechanism for disabling the click event behavior, but still enabling the custom tooltip behavior in the view events flyout. Do you see a way we could pass that logic here? Or if we split into 2 events maybe (1 for custom click event, 1 for custom hover event), be able to disable one and enable the other?
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.
The actions dont need to be defined here. The plugin listen for the trigger directly. If we have a standard contract for triggers (Which this Pr is adding), the plugin can simply listen for that trigger and do as they please.
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.
@amsiglan thanks. This makes sense to me in the context of the click event; I can defined a UIAction that listens for this trigger to open the view events flyout. But there's 2 things I'd like to clarify:
addVisEventSignalsToSpecConfig()
to only haveon
for the mouse events, and just omit the click event.tooltip()
function provided by vega to have a customized tooltip to render it OUI-style. It requires a lot of extra context, such as the container the vega view is in, the view itself, and other positioning parameters. I actually still think we may want to treat the tooltip action as just a param within the vis itself, that determines what tooltip html we render. The example I originally had is seen hereThere 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.
For 1. We don't need to disable the clicks, we could just check if the flyout is already open then just no-op in the event listener.
For 2. we will need to do a quick POC and check if the provided event has all the information we need. I see it has the view and the position related information
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.
How would you propose this? We don't persist global flyout state so how would we be able to make such a check within the UIAction?
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.
@ohltyler Why do we need to disable click? what is the interaction pattern that we want to achieve with that?
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.
So user can't infinitely re-open the flyout when clicking - details in the issue #3317
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.
Is this planned for functional testing, or can it be unit tested?