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

[filter bar] Init filter editing #5109

Merged
merged 5 commits into from
Oct 30, 2015
Merged

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Oct 13, 2015

Edit filters using a JSON editor. Validation is not performed on the filter object.

image

Closes #1583

@rashidkpc
Copy link
Contributor

Currently if the JSON is invalid, clicking Done will succeed, but all of the user's changes will be lost. Can we disable the done button if the JSON isn't valid at all?

queryFilter.updateFilter = function ({ source, model, type}) {
var editedFilter;
try {
editedFilter = JSON.parse(model);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should continuously validate the form and disable the Done button whenever the JSON isn't valid, then we shouldn't need the try/catch.

@rashidkpc
Copy link
Contributor

@jbudz Few comments, passing back to you

@rashidkpc rashidkpc assigned jbudz and unassigned rashidkpc Oct 26, 2015
@jbudz jbudz assigned rashidkpc and unassigned jbudz Oct 27, 2015
@jbudz
Copy link
Member Author

jbudz commented Oct 27, 2015

@rashidkpc Updated

@rashidkpc
Copy link
Contributor

Few more comments.

  1. Can you add a message next to the disabled Done button that says Could not parse JSON input
  2. If the filter is not one of the mapped types, the display of the filter stays the same, eg if you replace a filter that appears as say bytes: 30 with a bool filter containing something totally unrelated, the display will still say bytes: 30
  3. If you then refresh (cmd-R) the page, the filter bar will disappear, but the filter will still exist.

@tbragin
Copy link
Contributor

tbragin commented Oct 27, 2015

One big use case for filters is combining them into arbitrary predicate logic. While this is not possible directly from the UI perspective in this phase, one can achieve this by editing the filter directly (see examples below). The only drawback is that the filter name remains unchanged when you paste in a completely different filter.

screen shot 2015-10-27 at 11 25 33 am

Some future directions we’ve discussed post 4.3 include:

OR

{
  "bool": {
    "should": [
      {
        "term": {
          "geoip.country_name.raw": "Canada"
        }
      },
      {
        "term": {
          "geoip.country_name.raw": "China"
        }
      }
    ]
  }
}

AND

{
  "bool": {
    "must": [
      {
        "term": {
          "geoip.country_name.raw": "United States"
        }
      },
      {
        "term": {
          "geoip.city_name.raw": "New York"
        }
      }
    ]
  }
}

NOT

{
  "bool": {
    "must_not": [
      {
        "term": {
          "geoip.country_name.raw": "United States"
        }
      },
      {
        "term": {
          "geoip.country_name.raw": "Canada"
        }
      }
    ]
  }
}

@acs
Copy link
Contributor

acs commented Oct 27, 2015

@tbragin "User-configurable aliases for filter names" is the same than #5194?

@tbragin
Copy link
Contributor

tbragin commented Oct 27, 2015

@acs indeed, seems to be the same idea, i updated my comment above

@rashidkpc
Copy link
Contributor

Talked to @jbudz. This LGTM but I agree with @tbragin and @acs that it could be much more awesome with #5194, which should be a small change. We're going to try to get #5194 into 4.3.0

session.setTabSize(2);
session.setUseSoftTabs(true);

session.on('changeAnnotation', function checkIfValid() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, with external event emitters you need to call $scope.$digest() or something similar. Is that not the case here?

@spalger
Copy link
Contributor

spalger commented Oct 29, 2015

@jbudz functionally things work great, I do think that the editor belongs in a directive though, one that communicates it's validity and value to a containing form using an ngModelController. This way the transition to more complex forms will be smoother.

Another option is to put this transition off until we move to more complex editors. Either way, LGTM!

@rashidkpc
Copy link
Contributor

I agree it would be handy to have this in a directive that sets validity properly. I'm actually surpised ui-ace doesn't do it, but indeed, it does not: angular-ui/ui-ace#45. Tossing back to @jbudz

@rashidkpc rashidkpc removed their assignment Oct 29, 2015
function fromJSON(value) {
try {
value = JSON.parse(value);
var definedFilter = _.keys(value).length > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to an attribute option? This would allow us to reuse this validator elsewhere in the app, eg <textarea ng-model="myObject" require-keys=false>

<form role="form" name="editFilterForm" ng-submit="editDone()">
<div
json-input
require-keys=false
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be true

@spalger spalger merged commit 4d9b02f into elastic:master Oct 30, 2015
@tbragin tbragin mentioned this pull request Nov 3, 2015
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants