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.

Sign up for GitHub

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

[ML] Data Frame Analytics exploration page: filters improvements #91748

Merged
merged 4 commits into from
Feb 22, 2021
Merged

[ML] Data Frame Analytics exploration page: filters improvements #91748

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c9f9aa
exploration query bar filter reflects url query state on page load
alvarezmelissa87 Feb 17, 2021
150ee8b
wrap filter handler in debounce
alvarezmelissa87 Feb 17, 2021
a349738
add dep to memoized regex. update comment
alvarezmelissa87 Feb 19, 2021
0649f36
Merge branch 'master' into ml-ad-exploration-improvement
kibanamachine Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions ...cs/pages/analytics_exploration/components/exploration_query_bar/exploration_query_bar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
* 2.0.
*/

import React, { FC, useEffect, useState } from 'react';
import React, { FC, useEffect, useMemo, useState } from 'react';
import { EuiButtonGroup, EuiCode, EuiFlexGroup, EuiFlexItem, EuiInputPopover } from '@elastic/eui';
import { i18n } from '@kbn/i18n';

import { debounce } from 'lodash';
import { Dictionary } from '../../../../../../../common/types/common';
import { IIndexPattern } from '../../../../../../../../../../src/plugins/data/common/index_patterns';
import {
Expand Down Expand Up @@ -59,6 +60,33 @@ export const ExplorationQueryBar: FC<ExplorationQueryBarProps> = ({

const searchChangeHandler = (q: Query) => setSearchInput(q);

const regex = useMemo(() => new RegExp(`${filters?.columnId}\s?:\s?(true|false)`, 'g'), [
filters?.columnId,
]);

/**
* Restoring state from the URL once on load. If a filter option is active
* in the url set the corresponding options button to selected mode.
*/
useEffect(function updateIdToSelectedMap() {
if (filters !== undefined) {
const match: string[] | null = query.query.match(regex);
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a better way to parse the query? I think you can use esQuery or esKuery instead

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, here we just need to check the query string for the filter pattern - I'm not sure we need to parse it. Leaving as is for now as the regex check has been used for the quick filters since the beginning. Happy to revisit in the next release and do some testing around another way to parse the query.

let filterKeyInEffect: string | undefined;

if (match !== null && match[0].includes('true')) {
// set { training: true }
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 explain how the code knows this is about the training attribute? Or is this just an example and it's applicable to attributes in general?

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good question - the filter key property (filters are passed in as props) shows what field value the filter option corresponds to to be 'selected'

So for example the filter's columnId is ml.is_training'and the key for Training is training: true - so ml.is_training value would betrue. The key for Testing is testing: false meaning ml.is_training value should be false when this is selected.

So right now it would only work with filters where the column id value is a boolean. It is only used in dfa exploration right now, though so that should be fine as these are just quick filters and it is unlikely we would add quick filters for non boolean field values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh so "filter" refers to what's available as the buttons and not something derived from the free text query is that correct, that's why it can only be training?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, "filters" are what's available as the buttons and is passed into this component from the parent component.
training is just an example since that's the only current value is but it would be whatever is passed in as a filter.

🤔 Maybe this comment is more confusing than clarifying. Would it help to replace it with something like <filter_key_value: true> ? Or remove it entirely and add a comment for an example filter up by the filter type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Think it's fine to leave as is - just maybe update the comment above before the useEffect to emphasize this is about "restoring state from the URL once on load" or similar - I missed that and only now noticed the [] to make it trigger only once. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in a349738

filterKeyInEffect = Object.keys(filters.key).find((i) => filters.key[i] === true);
} else if (match !== null && match[0].includes('false')) {
// set { testing: true }
filterKeyInEffect = Object.keys(filters.key).find((i) => filters.key[i] === false);
}

if (filterKeyInEffect) {
setIdToSelectedMap({ [filterKeyInEffect]: true });
}
}
}, []);

/**
* Component is responsible for parsing the query string,
* hence it should sync submitted query string.
Expand Down Expand Up @@ -107,11 +135,10 @@ export const ExplorationQueryBar: FC<ExplorationQueryBarProps> = ({
});
};

const handleFilterUpdate = (optionId: string, currentIdToSelectedMap: any) => {
const debouncedHandleFilterUpdate = debounce((optionId: string, currentIdToSelectedMap: any) => {
let newQuery = '';
const filterValue = filters?.key[optionId];
const filterQueryString = `${filters?.columnId}:${filterValue}`;
const regex = new RegExp(`${filters?.columnId}\s?:\s?(true|false)`, 'g');

// Toggling selected optionId to 'off' - remove column id query from filter
if (currentIdToSelectedMap[optionId] === false) {
Expand Down Expand Up @@ -140,7 +167,7 @@ export const ExplorationQueryBar: FC<ExplorationQueryBarProps> = ({

setSearchInput(newSearchInput);
searchSubmitHandler(newSearchInput, true);
};
}, 200);

return (
<EuiInputPopover
Expand Down Expand Up @@ -189,7 +216,7 @@ export const ExplorationQueryBar: FC<ExplorationQueryBarProps> = ({
onChange={(optionId: string) => {
const newIdToSelectedMap = { [optionId]: !idToSelectedMap[optionId] };
setIdToSelectedMap(newIdToSelectedMap);
handleFilterUpdate(optionId, newIdToSelectedMap);
debouncedHandleFilterUpdate(optionId, newIdToSelectedMap);
}}
/>
</EuiFlexItem>
Expand Down