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

[Drilldowns] URL drilldown MVP #75450

Merged
merged 20 commits into from
Sep 4, 2020
Merged

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Aug 19, 2020

Summary

TLDR feature description

spec

Url drilldown is a new drilldown type which allows users to drilldown to internal or external URLs.
Url drilldown configuration includes destination URL and it supports dynamic variables.

Supported variables

URL drilldown will have 3 sources for variables:

  1. Global static variables, for now only kibanaBaseUrl.
  2. Context variables are dynamic and different depending on where drilldown is created and used. Now it includes {{context.panel.*}} variables extracted from embeddable.
  3. Event variables depend on a trigger context. These variables are dynamically extracted from the trigger context when drilldown is executed.

Subtle but important difference between context and event variables is that context variables have real values during creation. But in case of event variables we have to mock them for the user during creation.

Templating language

  • It uses handlebars with a set of custom helpers:
    • date. Uses moment and @kbn/datemath. example: {{date "now-15d" "DD MM YYYY"}}
    • rison. example {{rison context.filters}}. or {{rison filters=context.filters}}
    • json. example {{json context.filters}}. or {{json filters=context.filters}}

Reusability considerations

Current version relies on embeddable and supports only VALUE_CLICK_TRIGGER and RANGE_SELECT_TRIGGER.
In future we could expose extension points to support more triggers and provide more variables in a context. There was a POC with such extension points see details

In case solution needs a URL drilldown and wants more customisation then just supporting more triggers and providing custom variables, then we'd recommend to build custom URL drilldown and reuse our building blocks from ui_actions_enhanced.

Templating UI

Is very straightforward for now, but it does the job. We have a follow up issue: #69413
Validation is pretty good already.

Screenshot 2020-08-21 at 12 46 18

Checklist

Delete any items that are not applicable to this PR.

@Dosant Dosant changed the title [Drilldowns] URL drilldown [Drilldowns] URL drilldown MVP Aug 21, 2020
@streamich streamich mentioned this pull request Aug 21, 2020
24 tasks
@Dosant Dosant marked this pull request as ready for review August 21, 2020 14:41
@Dosant Dosant requested a review from a team August 21, 2020 14:41
@Dosant Dosant requested review from a team as code owners August 21, 2020 14:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

  1. Let's add the text Add variable in front of the icon for the context variable menu
  2. Change the switch to a checkbox for 'Open in new window' (sorry)
  3. Let's make the trigger picker full width

@Dosant
Copy link
Contributor Author

Dosant commented Aug 25, 2020

@mdefazio,

  1. Add variable text added
  2. EuiCheckbox used instead of switch
  3. Agreed offline to leave not full-width

@elastic-jb
Copy link

I'm looking at the Handlebar documentation and it mentions that I need to register a custom helper to use upper/lower case. In this case I am trying to go to the NBA.com team page (https://www.nba.com/teams/wizards) from my dashboard and the NBA URL values are case sensitive. The team names are capitalized in the data, so the page is silently redirecting away.

Handlebars.registerHelper('toLowerCase', function(str) {
return str.toLowerCase();
});

Is there a more direct way to do this? If not, we should consider some basic operations to add helpers for things like case, left/mid/right string, etc.

@Dosant
Copy link
Contributor Author

Dosant commented Sep 1, 2020

@elastic-jb,

  1. If we'd agree on a set of most useful functions, we could add them
    1.1. Shouldn't be a problem to add those in following prs
    1.2. We could consider using these helpers lib, but it seems that there are a lot of redundant stuff, and if possible, I think, I'd prefer to keep our api surface as lean as possible and avoid adding new dependencies. But we could pick helpers we think would be most useful for our scenarios.
    1.3 We discussed with @streamich that worth exposing registerHelper as extension point. So 3rd party plugin developers could add their own helpers to URL drilldown
  2. Other alternative you could try is to:
    2.1 change your data
    2.2. use scripted fields
  3. With URL drilldown there is always a workaround to create you own custom proxy server and do proper redirect from there (for such unfortunate cases)

@Dosant
Copy link
Contributor Author

Dosant commented Sep 2, 2020

We discussed offline with @elastic-jb to not rush with adding more helpers. JB, will try scripted fields for his example.

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM, tested out in chrome linux

…down-impl

# Conflicts:
#	x-pack/examples/ui_actions_enhanced_examples/public/dashboard_to_url_drilldown/index.tsx
Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Tested Locally in Chrome - URL drilldown behavior looks good to me.

Code LGTM.

Additionally, I agree with @streamich's ideas about the variable dropdown in a followup. I also wonder, though if the variable dropdown needs the moustache braces around each item, or if we could place the braces when a variable is clicked. Seems much cleaner to me:

Screen Shot 2020-09-02 at 11 34 53 AM

I am also wondering if we could revisit the error messaging on invalid variables and syntax.

Screen Shot 2020-09-02 at 11 45 00 AM

Screen Shot 2020-09-02 at 11 44 42 AM

I'm thinking that something a little more generic could be more user-friendly:
Invalid Format: "testingHello" does not exist or just Invalid Format

Overall, looks great and works well. This is going to be a huge addition to Kibana!

@Dosant Dosant mentioned this pull request Sep 2, 2020
@cchaos
Copy link
Contributor

cchaos commented Sep 2, 2020

Can I offer a component suggestion for the dropdown? I see that this list continues to grow and no overflow is being taken into consideration yet. We have a virtualized list component that can help with overflow, adds filter ability, etc. You can use EuiSelectable whose docs page shows an example of a popover usage. You don't have to use the icons either to signify selection, you can just handle the clicks.

Screen Shot 2020-09-02 at 13 38 52 PM

And the list items can be of any display you need.

@Dosant
Copy link
Contributor Author

Dosant commented Sep 3, 2020

@ThomThomson, thanks for review.

  1. I removed {{ .. }} 👍
  2. I agree that error messaging is far from ideal, but I am glad it is there :). In this MVP it just forwards error messages from handlebars. So there is no layer where we parse the expression ourselves to check the validity. I think for now it is a OK tradeoff.

@Dosant
Copy link
Contributor Author

Dosant commented Sep 3, 2020

@cchaos, thanks for the suggestion!

Screenshot 2020-09-03 at 13 14 54

@Dosant
Copy link
Contributor Author

Dosant commented Sep 3, 2020

@elasticmachine merge upstream

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 looking really good. Approve!

@Dosant
Copy link
Contributor Author

Dosant commented Sep 4, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
embeddableEnhanced 141 +131 10
uiActionsEnhanced 256 +127 129
total +258

page load bundle size

id value diff baseline
dashboardEnhanced 185.7KB +74.0B 185.6KB
embeddableEnhanced 92.1KB +66.9KB 25.2KB
uiActions 218.6KB +8.0B 218.6KB
uiActionsEnhanced 335.1KB +171.6KB 163.5KB
total +238.6KB

History

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

@Dosant Dosant merged commit e7d80e7 into elastic:master Sep 4, 2020
Dosant added a commit to Dosant/kibana that referenced this pull request Sep 4, 2020
Dosant added a commit that referenced this pull request Sep 4, 2020
@streamich streamich mentioned this pull request Sep 8, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Drilldowns Embeddable panel Drilldowns v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL Drilldown MVP
9 participants