embed visualization in tooltip #14102
Conversation
thomasneirynck
added
the
Feature:Visualizations
|
Hah, that's super cool |
|
amazing! |
|
Really cool! Maybe we can get a customer who was interested in this feature to take a look and give some feedback? cc @alexfrancoeur @nreese - how much effort do you think it would take to push it across the finish line? Is it something you see being done in addition to panel groupings, or would it take the place of that feature for 6.1? I wonder if people will end up wanting to interact with the visualization inside the tooltip. |
|
I like the idea. You'll have to handle a "loading icon" or something because some visualizations can take minutes. It would be cool if if you could specify a list of visualizations you want and when you interact with the chart in someway (click?), you get the list in a context menu that lets you pick which one you want. Kind of like an in-page drill down. |
|
@stacey-gammon I guess it would take about a week to polish things up for a review |
|
@thomasneirynck @ppisljar In the visualize editor - should |
There was a problem hiding this comment.
This is genuinely a great contribution 🤤
Since this is a POC, not much feedback on the code now. Also because #14146 could be something that will remove some of the boilerplate here.
But a few initial comments, in no particular order:
- To your comment, I don't think this should be a separate tab. Rather, we could instead add this to the
vislib_basic_options.htmldirective. It's somewhat of a goofy combination of tooltip-checkbox and legend position now, but we can format that a little better and just have two new sub-sections. Legend and Tooltip. - Embedding maps in the tooltip isn't very useful now, because we don't have autofit on the data. There's even a ticket for this Coordinate Maps: auto fit data bounds #2054, yikes!
- as discussed earlier, for the map itself to use this tooltip, it would have to use Kibana's tooltip not leaflet one. That doesn't need to be part of this PR imho, we can do that separately.
- as for @trevan's embed visualization in tooltip #14102. It's an intriguing idea to make the visualization itself toggle-able on the fly inside the toolip. But I think this is a good scope already. Multi-visualization is something we can explore later imho
- the preset width/height is a tough one. My initial gut feeling would be to invert this, that every visualization determines the best width/height for display in tooltip. But I can see the value of having it be configurable too. Getting this in the hands of end-users will probably determine this better.
- the reuse of the popup dom-element and vis make this actually quite snappy when hovering over chart-elements. For example, the tag-cloud doesn't completely become a pinball-machine when hovering around. nonetheless, animated visualizations, like tagcloud, are probably not the best to embed. I'd consider for example filtering out some visualizations, like tag-cloud.
- I ran into a few issues with jitter. e.g. tooltip is hovering, and then clicking a chart-element to send a filter makes the tooltip flash.
- cue the inception jokes, but the main visualization itself should probably be removed from the list of tooltip visualizations.
|
What a nice surprise! What else do you have up your sleeve @nreese!?! This is a great concept. All in all, I think it's a good idea and worth exploring further but we'd need to do a lot of cleaning up. Thoughts and feedback below (in no particular order).
|
|
|
nreese
changed the title
|
jenkins, test this |
1 similar comment
|
jenkins, test this |
| do { | ||
| const resp = await props.savedObjectsClient.find(findOptions); | ||
| const optionsFromPage = resp.savedObjects | ||
| .filter((savedObject) => { |
There was a problem hiding this comment.
I would also filter out the visualization itself.
This only occurs if the visualization is already saved, but it shows up in the list. It doesn't really add anything.
e.g.:
| </div> | ||
|
|
||
| <tooltip-options |
There was a problem hiding this comment.
imho, I'd be in favor of putting a subheader above all the tooltip options and legend position. Right now, the section kinds of blends in with the rest:
There was a problem hiding this comment.
Tooltip settings are now under the collapsible header Tooltip Settings
| import React from 'react'; | ||
| import Select from 'react-select'; | ||
|
|
||
| const visTypeExcludeList = [ |
There was a problem hiding this comment.
what are your thoughts on flipping this? Every vis must set a property isEmbeddableInTooltip to true, iso having this exclusion list. The biggest advantage of that approach is that new visualization (e.g. 3rd party plugins) won't be embeddable by default. I
| height: getHeight() | ||
| }); | ||
| $popup.empty(); | ||
| $popup.append($visEl); |
There was a problem hiding this comment.
the default legend-width (https://github.com/thomasneirynck/kibana/blob/e91c99f2b236339c0f12819974a9ce9d9f18503f/src/ui/public/vislib/styles/_legend.less#L44-L44) makes the legend look too wide with wasted space to the right when embedded in the tooltip.
Consider overriding that rule with width: auto when inside a tooltip.
There was a problem hiding this comment.
auto will not work because then long labels will make the legend cover the entire tooltip. I shrank the value to 75px for legends embedded in tooltips. Are you ok with that solution?
| return from.find(handler => handler.name === name).handler; | ||
| } | ||
|
|
||
| return function (parentVis) { |
There was a problem hiding this comment.
I like the expansion of this type (embeddedTooltipFormatter/hierarchicalTooltipFormatter), but the implementation is very Crockfordian. imho, given we expand on it, I'd consider using the simple class syntax.
so something like:
class VisualizationTooltipFormatter {
constructor(parentVis) {
}
formatTooltipContents(event) {
}
cleanUp(){
}
}
There was a problem hiding this comment.
As discussed, I opened an issue to resolve this latter
| * Get content container for Tooltip. | ||
| * Tooltips with the same id share a content container. | ||
| */ | ||
| Tooltip.prototype.getContentContainer = function () { |
There was a problem hiding this comment.
Maybe I'm missing something, but do we need a new container for each tooltip type?
There was a problem hiding this comment.
The old implementation just concatenated a bunch of HTML strings together. Each mousemove event destroyed the existing HTML and generated new HTML.
This is no longer possible since the HTML can now be generated by asynchronous calls. Putting each tooltip in its own container helped make the implementation more straightforward.
| .then(requestHandlerResponse => { | ||
| return responseHandler(vis, requestHandlerResponse); | ||
| }) | ||
| .then(resp => { |
There was a problem hiding this comment.
what happens if formatter#cleanUp is called before this handler is executed?
There was a problem hiding this comment.
formatter#cleanUp is fired when there is a mouseleave event. The $popup is also removed from the DOM on mouseleave. Therefore - the if statement if (localFetchTimestamp === fetchTimestamp && $popup && $popup.length > 0) will return false and the response will be ignored.
The responseHandler is not effected by cleanUp and can process the response. The above mentioned logic will handle everything.
There was a problem hiding this comment.
cleanUp now sets fetchTimestamp to expired so regardless if $popup has been removed or not, the if block will return false if cleanUp is called before the response is returned
| self.contentContainer.empty(); | ||
| } else { | ||
| const events = self.events ? self.events.eventResponse(d, i) : d; | ||
| self.contentContainer.html(self.formatter(events)); |
There was a problem hiding this comment.
I'd consider changing this self.formatter method instead of returning a html-string, to having it return a DOM-node, or append to a DOM-node.
This will avoid having to create dynamically generate IDs and then querying the DOM for the container-node because you can just keep a reference to the node directly. (see e.g. the work-around of creating the executionId just so we can find the node in embeddedTooltipFormatter).
There was a problem hiding this comment.
As discussed, this is a good idea but should be done outside of the scope of this PR
|
@thomasneirynck I should have addressed all concerns highlighted by your review |
|
jenkins, test this |
1 similar comment
|
jenkins, test this |
|
@nreese just checked out the latest and had some additional feedback. These comments are not in any prioritized order. We seem to be missing Timelion and TSVB visualization types. Neither visualization types show up in the drop down. Given that they are available when creating a new visualization and are timeseries specific, as a user I'd expect them to be viable options. Is there a technical limitation here? Can we get an official loading indicator from design? I see You can't interact with the legend at all. Any way to hide the collapsible button or is it more trouble than it's worth? Errors are not descriptive enough - what's the min/max value for a tooltip? We are also only visualization the error with color. We'll need to provide an icon and/or text for accessibility purposes.
Expanding / Collapsing Tooltip options This option is not keyboard accessible, tabbing skips right over it. The tool tip visualization does not seem to adhere to user defined intervals. I created a "scatter chart" with second interval expecting to hover and see more data points than the aggregation. However, the interval seems to be stuck with "auto" and is only providing one data point. Current behavior: Expected number of data points: There still seems to be a lot of wasted space with some visualizations Should we provide a warning for the max number of rows either when creating the visualization or on hover? Users should understand that they aren't seeing the full table if a table is used. We also seem to be cutting rows off, any way we can dynamically size as we would a chart? The X on selection seems to overlap the name of a visualization This is a bit of an after thought, but it's possible users may want to see the metric and a visualization. Thoughts? |
|
@stacey-gammon @thomasneirynck @alexfrancoeur Should the user even be allowed to set the embedded tooltip size? Maybe the size should just always be 40% of the window size? What do you think? |
|
@nreese in hindsight, yes, I agree that we can start with defaulting %40. If people start asking for more control, we can give that control to them later. But don't feel strongly about this, so I'll agree with majority. @alexfrancoeur imho I would hold off on showing both metrics and visualizations. could be introduced if people ask for it. |
|
I agree with @thomasneirynck, I'm comfortable with providing defaults and less options in the first iteration. |
…es and tooltip max-width CSS setting
…unneeded _msearch requests
…oid refactor since same formatter instance is used even though new instance of tooltip is created
…ad of hard coded black list
…arCollapsibleTitle
… of browser window
|
This is awesome. |
|
Having embedded visualizations in the tooltip is an awesome feature for business analytics dashboard. we using it on maps, but have it in any visualization would be even better. +1 |
|
@nreese any plan to bring this back? |
There is an open issue about tooltip aggregations. I thought it might be interesting to explore solving this issue by allowing the tooltip to contain a visualization that is further filtered by the bucket key(s) of the moused over element.
The current implementation allows for embedding visualizations in tooltips for
PieandVertical Barvisualizations.This is a very rough implementation to see if such a solution is possible and gauge the level of effort for a fully fleshed out solution and get feedback and determine if this is an idea worth pursuing.
@elastic/kibana-visualizations @elastic/kibana-sharing What are your thoughts? Should more effort be put into this? Does this complement drill down links?