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

[discuss][Drilldowns][URL drilldown] Expose "event.row" in VALUE_CLICK_TRIGGER #79157

Closed
wants to merge 1 commit into from

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Oct 1, 2020

Summary

Problem

Consider the table:

Screenshot 2020-10-01 at 18 14 56

With current variables setup when drilling down in to cell in the table (e.g. "Venice") it was not possible to access data from the whole row. (No access to "IT" or "4").

Solution

This pr exposes {{event.row}}.

Screenshot 2020-10-01 at 18 14 50

And it allows to build a URL that uses the whole row:

Screenshot 2020-10-01 at 18 15 39

Caveats

  1. Because of how preview with mock variables work, I had to weaken the validation for saving the Drilldown. number of items in row could be large and we won't cover all this variables with mocks. to be able to save something like: {{event.row.[100]}} I had to weaken editor validation. In runtime it still won't be compatible if variable is not available. [discuss][Drilldowns] More specific triggers and trigger context samples #76226

  2. Do we need anything else to be exposed? Columns? Clicked rowIndex? columnIndex? In what format?

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@Dosant Dosant added Feature:Drilldowns Embeddable panel Drilldowns Team:AppArch labels Oct 1, 2020
@Dosant Dosant requested a review from mdefazio October 1, 2020 16:23
@elastic-jb
Copy link

This looks really good. Is there something we can do to respect the formatting on the chart or should we add a helper for formatting decimal places?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
uiActionsEnhanced 320.7KB 321.0KB +397.0B
urlDrilldown 18.7KB 19.4KB +698.0B
total +1.1KB

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Oct 5, 2020

Is there something we can do to respect the formatting on the chart or should we add a helper for formatting decimal places?

I think we should consider adding a helper for this. #77758

@Dosant
Copy link
Contributor Author

Dosant commented Oct 7, 2020

Discussed with @ppisljar:

Short-term this makes sense. But long-term it could be changed until we decide what exactly we want to do with this triggers. like, should we rather than index for columns use column names ? like row.count and row.city. but then this again needs us to know more about data when we are creating this ... (but not having this makes it easier to write invalid urls)

We agreed this should also be considered as part of spike about #76226 planned for 7.11

Copy link

@elastic-jb elastic-jb left a comment

Choose a reason for hiding this comment

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

This is definitely what we need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants