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

Url template editor #88577

Merged
merged 73 commits into from
Feb 15, 2021
Merged

Url template editor #88577

merged 73 commits into from
Feb 15, 2021

Conversation

streamich
Copy link
Contributor

@streamich streamich commented Jan 18, 2021

Summary

Closes #69413
Closes #83328

This PR replaces URL Drilldown textarea for URL template editing with Monaco editor. This enables:

  • Syntax highlighting for Handlebars variables.
  • Syntax highlighting for different URL parts.
  • Autocompletion for Handlebars variables.
  • Description text for each variable inside autocomplete popup.
  • Supports dark mode.
  • Previews variable values in autocomplete popup.
  • Sample URL instead of placeholder.

Simple URL:

image

With query params:

image

Complex long link:

image

With if-statement:

image

Autocompletion:

image

Description about each variable:

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@streamich streamich requested review from Dosant and ppisljar February 9, 2021 09:17
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.

This looks awesome. Just some super tiny nits regarding sass variables and copy.

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

operations - storybook aliases LGTM

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.

autocompletion is a bit annoying, is it possible to tweak this ?

  • autocompletion auto pops up while writing your URL at any point, in between url but also when inside {{}} code brackets writing my if statement. if selected there it will insert nested {{}} which is probably not what we want. I would expect autocompletion to either not pop up when i am inside {{}} or to correctly insert variable name in that case (without nested {{}})
  • there is no autocompletion on handlebars operators, would that be possible to add, so that when I am inside {{}} i could see what's possible ?
  • accessing documentation is only possible with keyboard+mouse combo. mouse over the dropdown entry doenst show the short description and info icon (this is only possible with keyboard). there seems to be no way to select info icon (expand info) with keyboard only (clicking the info icon is only possible with mouse). also there seems to be no way to close the info popup with keyboard (only possible with mouse)

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Nice work! Was surprised to see value where possible as part of documentation!

Some feedback I think we could try to improve:

  1. When I typed {{ and then do auto-completion inside it results in {{{{. Would it be possible to avoid it?
    Screenshot 2021-02-10 at 11 44 23
    Screenshot 2021-02-10 at 11 44 26

  2. In older implementation longer URLs wrapped and this was convenient, is it possible to do something similar? I think currently It is more difficult to work with longer URLs:
    Before:
    Screenshot 2021-02-10 at 12 30 23
    After:
    Screenshot 2021-02-10 at 12 50 39

  3. To my eye editor doesn't have enough contrast with the flyout background. Maybe it needs a border or darker background? cc @mdefazio

  4. URL highlight breaks for complex URLs, for example, like we have in Kibana:
    try this one:

{{kibanaUrl}}/app/discover#/?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:'{{event.from}}',to:'{{event.to}}'))&_a=(columns:!(_source),filters:{{rison context.panel.filters}},index:'{{context.panel.indexPatternId}}',interval:auto,query:(language:{{context.panel.query.language}},query:'{{context.panel.query.query}}'),sort:!())

Screenshot 2021-02-10 at 12 54 30

if fixing this is a question of adjusting regex, then it would definitely worth improving 👍

  1. Looks like URL dropdown editor stuff was part of a main bundle :( (tech debt). With this pr this piece is growing significantly. Would it be possible to move some of that into async chunk? If it is not straightforward, then we should create an issue.

  2. One of the functional test is still failing? Didn't look into that. Just calling out, not sure if a legit regression.

@mdefazio
Copy link
Contributor

To my eye editor doesn't have enough contrast with the flyout background. Maybe it needs a border or darker background?

This is a tough balance—if we darken the background it will likely not have enough contrast with the text inside it. Perhaps a slightly darker border as you mentioned might help better define where the code editor is within the flyout. Though I don't know that we do this elsewhere. Maybe wait and see if others mention it as well?

@streamich
Copy link
Contributor Author

streamich commented Feb 12, 2021

@mdefazio I've implemented both of your suggestions.

@ppisljar still looking into some of your comments, some answers:

  • The autocompletion pops up when you press Cmd + Space or Monaco editor allows you to specify special characters on which the autocompletion pops up. Currently, those characters are specified as ['{', '/', '?', '&', '=']. I can possibly tweak it to make it appear less often, will take a look.
  • I have fixed the nested {} issue, so now if you have {|} or {{|}} it will not insert extra brackets.
  • I'll see what I can do about autocompletion once you are inside the handlebars tokens, currently it has no special treatment for that.
  • You can close the auto-completion popup with Esc button. The last meeting we discussed that showing auto-completion on mouse hover is not mandatory, but I'll see what I can do.

@Dosant all your comments should be addressed now:

  • Fixed autocompletion with extra {{{{.
  • Added line-wrapping.
  • I haven't done anything for changing the background of the editor, as suggested by Michael let's see what others think. And another thing to note is that the Monaco editor component is shared, so such CSS changes will influence everyone.
  • I've solved the complex URL highlighting with a quick tweak by removing the green highlighting for the query parameter values, as it wasn't really useful anyway.
  • I've made the drilldown config component load lazily.
  • Fixed the functional test.

@streamich
Copy link
Contributor Author

streamich commented Feb 15, 2021

Zoomed with @ppisljar, we agreed that current PR is good to be merged and in a separate PR I will work to improve:

  • Autocompletion should not open once user types regular characters after a special character, for example, after typing c after ., in www.example.c|.
  • Autocompletion should not open, or open more smartly, once user's cursor is in a block of Handlebars variable.

Update: I've figured it out, disabling autocompletion in those cases could be achieved with a setting for Monaco editor. I'll push a commit to this PR.

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

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

Thanks for addressing earlier feedback 🚀

);
};

public readonly CollectConfig = reactToUiComponent(this.ReactCollectConfig);

public readonly createConfig = () => ({
url: { template: '' },
url: {
template: 'Example: https://example.com/?{{event.key}}={{event.value}}',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that because of how we don't have a real placeholder, but a "default" URL instead it is not a good idea to have Example prefix 🤔

Because now our default URL template is invalid

cc @mdefazio

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @Dosant, it feels more natural to have a real URL as that is not a placeholder but an example. I've reverted back to have it without "Example: " prefix, unless you have a strong preference @mdefazio.

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.

Presentation team changes LGTM. Code only review. PR is looking awesome!

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.

Thanks for making the edits!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
kibanaReact 321 334 +13
uiActionsEnhanced 120 124 +4
urlDrilldown 29 33 +4
total +21

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
kibanaReact 342.8KB 342.9KB +150.0B
uiActionsEnhanced 0.0B 12.3KB +12.3KB
total +12.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kibanaReact 119.4KB 127.3KB +8.0KB
uiActionsEnhanced 226.3KB 217.5KB -8.8KB
urlDrilldown 42.5KB 52.2KB +9.7KB
total +8.9KB
Unknown metric groups

async chunk count

id before after diff
uiActionsEnhanced 0 1 +1

History

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

@streamich streamich merged commit 8ed1c3c into elastic:master Feb 15, 2021
streamich added a commit that referenced this pull request Feb 15, 2021
* feat: 🎸 set up Storybook for URL template editor

* feat: 🎸 add basic syntax highlighting

* feat: 🎸 add autocompletion example

* feat: 🎸 add Handlebars language

* fix: 🐛 first register language

* feat: 🎸 add url and handlebars language parsing

* feat: 🎸 use simple Handlebars language

* refactor: 💡 move <VariablePopover> to a separate file

* feat: 🎸 add Monaco editor to URL drilldown

* feat: 🎸 remove editor line numbers

* feat: 🎸 allow user to provide Handlebars variables

* feat: 🎸 wire in URL drilldown variables into Monaco editor

* feat: 🎸 add metadata to event level variables

* feat: 🎸 allow to specify Handlebars variable kind

* feat: 🎸 add global variables to autocompletion

* refactor: 💡 restructure event and context variable code

* feat: 🎸 sort variables by scope group

* feat: 🎸 add meta information to context variables

* docs: ✏️ use correct variable labels

* feat: 🎸 fix component demo props

* feat: 🎸 improve highlighting of URL parts

* feat: 🎸 improve syntax highlighting colors

* feat: 🎸 improve highlighting colors

* feat: 🎸 add color to url query parameter key

* feat: 🎸 improve visual layout url editor

* feat: 🎸 highlight URL slashes with light color

* feat: 🎸 connect URL editor to state

* feat: 🎸 tweak URL parameter colors

* feat: 🎸 improve URL schema color

* feat: 🎸 insert variables on click in variable dropdown

* fix: 🐛 fix unit tests and translation

* test: 💍 fix drilldown tests after refactor

* feat: 🎸 add dark mode support to URL template editor

* test: 💍 fix URL drilldown test after adding dark mode support

* fix: 🐛 use text color which can be converted to dark mode

* test: 💍 fill in URL template in monaco editor

* fix: 🐛 fix translation key

* chore: 🤖 update license headers

* chore: 🤖 update license headers

* feat: 🎸 preview values of global variables

* feat: 🎸 preview values of context variables

* chore: 🤖 fix url editor Storybook config

* fix: 🐛 make translation key unique

* feat: 🎸 stop Esc key propagation in URL editor

* feat: 🎸 reduce editor height

* feat: 🎸 set example URL once URL drilldown is created

* feat: 🎸 add word wrapping to URL editor

* feat: 🎸 use EUI variable in SCSS

* feat: 🎸 add "Example: " prefix to default template

* feat: 🎸 do not insert extra brackets

* feat: 🎸 make URL param values same color as text

* perf: ⚡️ make URL drilldown config component lazy loaded

* test: 💍 remove default URL drilldown template

* fix: 🐛 disable autocompletion popup while typing

* style: 💄 don't use "Example: " prefix in default URL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants