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

[Lens] Filters aggregation #75635

Merged
merged 43 commits into from
Sep 10, 2020
Merged
Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
e82e536
checkpoint
mbondyra Aug 21, 2020
47ba9df
filters aggregation tweaks and types
mbondyra Aug 24, 2020
38fb236
refactoring
mbondyra Aug 24, 2020
be5ae8d
style fixes
mbondyra Aug 24, 2020
8d1227d
addressing feedback
mbondyra Aug 25, 2020
66aada5
tests added
mbondyra Aug 26, 2020
030b5f7
types
mbondyra Aug 26, 2020
f21b11f
some of easier feedback addressed
mbondyra Aug 27, 2020
9f3f4a8
Merge commit '03002453d3d90831670c5215a613a52bd0306f00' into lens/fil…
mbondyra Sep 2, 2020
0c85af1
change of the popovers behavior
mbondyra Sep 2, 2020
07ce55a
error validation
mbondyra Sep 3, 2020
6fbac67
continue with feedback
mbondyra Sep 4, 2020
f97e5f6
after meeting fixes
mbondyra Sep 4, 2020
ccd9ed7
update tests
mbondyra Sep 4, 2020
becb89a
Merge commit 'fc22c7d9fb8aa767dcb5136f11451da0561a50a7' into lens/fil…
mbondyra Sep 4, 2020
c659183
added termstofilters behavior
mbondyra Sep 4, 2020
9537d67
Merge commit '05ac18a4a222e491c555a70f642787907b042dbc' into lens/fil…
mbondyra Sep 7, 2020
2503ba0
Merge commit '5a1d0753c07c64717f77a6f3c2004eabf285bd0a' into lens/fil…
mbondyra Sep 7, 2020
c3f3ded
fixing types
mbondyra Sep 7, 2020
9f2bd40
tests fixes
mbondyra Sep 8, 2020
e1fe770
Merge commit 'eecb0e59220c7c29811842a7911b6b2a3b872b5e' into lens/fil…
mbondyra Sep 8, 2020
fdbf1ae
remove magic mapping type
flash1293 Sep 8, 2020
acbffdb
Merge pull request #3 from flash1293/lens/operation-type-cleanup
mbondyra Sep 8, 2020
5c4c1d5
revert (simplify) if statements to ternary
mbondyra Sep 8, 2020
6e0b3f3
types again
mbondyra Sep 8, 2020
c0d36e6
defaultFilter fix
mbondyra Sep 8, 2020
a931828
Merge commit 'e827a6761e1667ea8b9ff1f10603849bc7219f91' into lens/fil…
mbondyra Sep 8, 2020
98da724
Merge commit '728dfb4b6bd6fd9ee1c736132b4b7f96fcccb70e' into lens/fil…
mbondyra Sep 8, 2020
2f6339d
Fixing up KWL bar to support `isInvalid`
Sep 8, 2020
03c00d8
Fixing up layout of filter agg button
Sep 8, 2020
3216553
types fixes
mbondyra Sep 9, 2020
c3de990
Merge branch 'master' into lens/filters_aggregation
mbondyra Sep 9, 2020
127e539
refactor: complicated condition
mbondyra Sep 9, 2020
81c4080
Merge branch 'lens/filters_aggregation' of github.com:mbondyra/kibana…
mbondyra Sep 9, 2020
3a29c0f
Fixing word-wrap and adding more titles
Sep 9, 2020
b539a0f
Cleanup
Sep 9, 2020
c18c658
Update x-pack/plugins/lens/public/editor_frame_service/editor_frame/c…
mbondyra Sep 9, 2020
871ec5b
Revert "Update x-pack/plugins/lens/public/editor_frame_service/editor…
Sep 9, 2020
5cf7490
refactor: copy
mbondyra Sep 10, 2020
e95af12
added tests
mbondyra Sep 10, 2020
eebbbdb
type
mbondyra Sep 10, 2020
e0058e2
Merge commit '100afab3085e735fa0624a07a54a7f752d92935f' into lens/fil…
mbondyra Sep 10, 2020
2978769
useDebounce properly this time
mbondyra Sep 10, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
<b>Signature:</b>

```typescript
QueryStringInput: React.FC<Pick<Props, "query" | "prepend" | "size" | "className" | "placeholder" | "onChange" | "onBlur" | "onSubmit" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>
QueryStringInput: React.FC<Pick<Props, "query" | "prepend" | "size" | "className" | "placeholder" | "onChange" | "onBlur" | "onSubmit" | "isInvalid" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>
```
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,6 @@
border-radius: 0;
border-left-width: 0;
}

.kuiLocalSearchAssistedInput {
display: flex;
flex: 1 1 100%;
position: relative;
}

/**
* 1. em used for right padding so documentation link and query string
* won't overlap if the user increases their default browser font size
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1479,7 +1479,7 @@ export interface QueryStateChange extends QueryStateChangePartial {
// Warning: (ae-missing-release-tag) "QueryStringInput" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
//
// @public (undocumented)
export const QueryStringInput: React.FC<Pick<Props_3, "query" | "prepend" | "size" | "className" | "placeholder" | "onChange" | "onBlur" | "onSubmit" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>;
export const QueryStringInput: React.FC<Pick<Props_3, "query" | "prepend" | "size" | "className" | "placeholder" | "onChange" | "onBlur" | "onSubmit" | "isInvalid" | "indexPatterns" | "dataTestSubj" | "screenTitle" | "disableAutoFocus" | "persistedLog" | "bubbleSubmitEvent" | "languageSwitcherPopoverAnchorPosition" | "onChangeQueryInputFocus">>;

// @public (undocumented)
export type QuerySuggestion = QuerySuggestionBasic | QuerySuggestionField;
Expand Down
23 changes: 15 additions & 8 deletions src/plugins/data/public/ui/query_string_input/_query_bar.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,37 @@
border-right: none !important;
}

.kbnQueryBar__textareaWrap {
overflow: visible !important; // Override EUI form control
display: flex;
flex: 1 1 100%;
position: relative;
}

.kbnQueryBar__textarea {
z-index: $euiZContentMenu;
resize: none !important; // When in the group, it will autosize
height: $euiSizeXXL;
height: $euiFormControlHeight;
// Unlike most inputs within layout control groups, the text area still needs a border.
// These adjusts help it sit above the control groups shadow to line up correctly.
padding-top: $euiSizeS + 3px !important;
transform: translateY(-2px);
padding: $euiSizeS - 1px;
padding: $euiSizeS;
padding-top: $euiSizeS + 3px;
transform: translateY(-1px) translateX(-1px);

&:not(:focus) {
&:not(:focus):not(:invalid) {
@include euiYScrollWithShadows;
}

&:not(:focus) {
white-space: nowrap;
overflow-y: hidden;
overflow-x: hidden;
border: none;
box-shadow: none;
}

// When focused, let it scroll
&:focus {
overflow-x: auto;
overflow-y: auto;
width: calc(100% + 1px); // To overtake the group's fake border
white-space: normal;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import React, { Component, RefObject, createRef } from 'react';
import { i18n } from '@kbn/i18n';

import classNames from 'classnames';
import {
EuiTextArea,
Expand Down Expand Up @@ -63,6 +64,7 @@ interface Props {
dataTestSubj?: string;
size?: SuggestionsListSize;
className?: string;
isInvalid?: boolean;
}

interface State {
Expand Down Expand Up @@ -591,6 +593,7 @@ export class QueryStringInputUI extends Component<Props, State> {
'euiFormControlLayout euiFormControlLayout--group kbnQueryBar__wrap',
this.props.className
);

return (
<div className={className}>
{this.props.prepend}
Expand All @@ -607,7 +610,7 @@ export class QueryStringInputUI extends Component<Props, State> {
>
<div
role="search"
className="euiFormControlLayout__childrenWrapper kuiLocalSearchAssistedInput"
className="euiFormControlLayout__childrenWrapper kbnQueryBar__textareaWrap"
ref={this.queryBarInputDivRefInstance}
>
<EuiTextArea
Expand Down Expand Up @@ -651,6 +654,7 @@ export class QueryStringInputUI extends Component<Props, State> {
}
role="textbox"
data-test-subj={this.props.dataTestSubj || 'queryInput'}
isInvalid={this.props.isInvalid}
>
{this.getQueryString()}
</EuiTextArea>
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/data/public/ui/typeahead/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ export const SUGGESTIONS_LIST_REQUIRED_BOTTOM_SPACE = 250;
* A distance in px to display suggestions list right under the query input without a gap
* @public
*/
export const SUGGESTIONS_LIST_REQUIRED_TOP_OFFSET = 2;
export const SUGGESTIONS_LIST_REQUIRED_TOP_OFFSET = 1;
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ export class SuggestionsComponent extends Component<Props> {
const StyledSuggestionsListDiv = styled.div`
${(props: { queryBarRect: DOMRect; verticalListPosition: string }) => `
position: absolute;
z-index: 4001;
left: ${props.queryBarRect.left}px;
width: ${props.queryBarRect.width}px;
${props.verticalListPosition}`}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
@import 'config_panel';
@import 'dimension_popover';
@import 'layer_panel';
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@
min-height: $euiSizeXXL;
}

.lnsLayerPanel__anchor {
width: 100%;
}

.lnsLayerPanel__dndGrab {
padding: $euiSizeS;
}

.lnsLayerPanel__styleEditor {
width: $euiSize * 30;
padding: $euiSizeS;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,10 @@
display: block;
word-break: break-word;
}

// todo: remove after closing https://github.com/elastic/eui/issues/3548
.lnsDimensionPopover__fixTranslateDnd {
// sass-lint:disable-block no-important
transform: none !important;
}

Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import './dimension_popover.scss';
mbondyra marked this conversation as resolved.
Show resolved Hide resolved

import React from 'react';
import { EuiPopover } from '@elastic/eui';
Expand Down Expand Up @@ -31,6 +32,7 @@ export function DimensionPopover({
<EuiPopover
className="lnsDimensionPopover"
anchorClassName="lnsDimensionPopover__trigger"
panelClassName="lnsDimensionPopover__fixTranslateDnd"
isOpen={
popoverState.isOpen &&
(popoverState.openId === accessor || (noMatch && popoverState.addingToGroupId === groupId))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export function BucketNestingEditor({
value,
text: c.label,
fieldName: hasField(c) ? fieldMap[c.sourceField].displayName : '',
operationType: c.operationType,
}));

if (!column || !column.isBucketed || !aggColumns.length) {
Expand All @@ -61,6 +62,36 @@ export function BucketNestingEditor({
}
}

// todo: move the copy to operations
const topLevelCopy: Record<string, string> = {
mbondyra marked this conversation as resolved.
Show resolved Hide resolved
terms: i18n.translate('xpack.lens.indexPattern.groupingOverallTerms', {
defaultMessage: 'Overall top {field}',
values: { field: fieldName },
}),
filters: i18n.translate('xpack.lens.indexPattern.groupingOverallFilters', {
defaultMessage: 'Search query overall',
}),
date_histogram: i18n.translate('xpack.lens.indexPattern.groupingOverallDateHistogram', {
defaultMessage: 'Top values for each {field}',
values: { field: fieldName },
}),
};

const bottomLevelCopy: Record<string, string> = {
terms: i18n.translate('xpack.lens.indexPattern.groupingSecondTerms', {
defaultMessage: 'Top values for each {target}',
values: { target: target.fieldName },
}),
filters: i18n.translate('xpack.lens.indexPattern.groupingSecondFilters', {
defaultMessage: 'Search query for each {target}',
values: { target: target.fieldName },
}),
date_histogram: i18n.translate('xpack.lens.indexPattern.groupingSecondDateHistogram', {
defaultMessage: 'Overall top {target}',
values: { target: target.fieldName },
Copy link
Contributor

Choose a reason for hiding this comment

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

This phrasing has bothered me for a long time, and if you feel like it's related feel free to make a change here. It's probably a separate PR though. #76038

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'll open a PR for this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, this whole grouping section's copy is not just confusing to read by the grammar is not great and I know that's because of the re-use of the field name, but I think we need phrasing or formatting that considers this better. Also the option for this type of agg shows as "Top values of each Records". I think "Records" here is incorrect?

}),
};

return (
<>
<EuiHorizontalRule margin="m" />
Expand All @@ -73,34 +104,14 @@ export function BucketNestingEditor({
<EuiRadio
id={generator('topLevel')}
data-test-subj="indexPattern-nesting-topLevel"
label={
column.operationType === 'terms'
? i18n.translate('xpack.lens.indexPattern.groupingOverallTerms', {
defaultMessage: 'Overall top {field}',
values: { field: fieldName },
})
: i18n.translate('xpack.lens.indexPattern.groupingOverallDateHistogram', {
defaultMessage: 'Top values for each {field}',
values: { field: fieldName },
})
}
label={topLevelCopy[column.operationType]}
checked={!prevColumn}
onChange={toggleNesting}
/>
<EuiRadio
id={generator('bottomLevel')}
data-test-subj="indexPattern-nesting-bottomLevel"
label={
column.operationType === 'terms'
? i18n.translate('xpack.lens.indexPattern.groupingSecondTerms', {
defaultMessage: 'Top values for each {target}',
values: { target: target.fieldName },
})
: i18n.translate('xpack.lens.indexPattern.groupingSecondDateHistogram', {
defaultMessage: 'Overall top {target}',
values: { target: target.fieldName },
})
}
label={bottomLevelCopy[column.operationType]}
checked={!!prevColumn}
onChange={toggleNesting}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@ export function PopoverEditor(props: PopoverEditorProps) {
compatibleWithCurrentField ? '' : ' incompatible'
}`,
onClick() {
// todo: when moving from terms agg to filters, we want to create a filter `$field.name : *`
Copy link
Contributor

Choose a reason for hiding this comment

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

A quick option to do this in a clean way is to have a property on the operation definition acceptsColumn(otherColumn: IndexPatternColumn): boolean which is called instead of putting that logic here. The specific implementation would be part of the filters operation definition.

I'm fine with not doing that now, in that case let's create a technical debt issue for it.

Copy link
Contributor Author

@mbondyra mbondyra Sep 9, 2020

Choose a reason for hiding this comment

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

The problem here is that other columns are already accepted if the field is compatible with them (the fact they are not accepted was the misconception I got from checking the previousColumn type we talked about async). In our case, top values and filters have totally non-compatible fields set. I think this has to be completely changed anyway once we allow creating fieldless columns. I'll create an issue, but without proposal for implementation, because it depends on fieldless column. Feel free to add some details if you have any ideas.
#77017

// it probably has to be re-thought when removing the field name.
const isTermsToFilters =
selectedColumn?.operationType === 'terms' && operationType === 'filters';

if (!selectedColumn || !compatibleWithCurrentField) {
const possibleFields = fieldByOperation[operationType] || [];

Expand All @@ -186,7 +191,7 @@ export function PopoverEditor(props: PopoverEditorProps) {
trackUiEvent(`indexpattern_dimension_operation_${operationType}`);
return;
}
if (incompatibleSelectedOperationType) {
if (incompatibleSelectedOperationType && !isTermsToFilters) {
setInvalidOperationType(null);
}
if (selectedColumn.operationType === operationType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ export function getIndexPatternDatasource({
data,
savedObjects: core.savedObjects,
docLinks: core.docLinks,
http: core.http,
}}
>
<IndexPatternDimensionEditor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ export const cardinalityOperation: OperationDefinition<CardinalityIndexPatternCo
sourceField: field.name,
isBucketed: IS_BUCKETED,
params:
previousColumn && previousColumn.dataType === 'number' ? previousColumn.params : undefined,
previousColumn?.dataType === 'number' &&
mbondyra marked this conversation as resolved.
Show resolved Hide resolved
previousColumn.params &&
'format' in previousColumn.params
? previousColumn.params
: undefined,
};
},
toEsAggsConfig: (column, columnId) => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ export const countOperation: OperationDefinition<CountIndexPatternColumn> = {
scale: 'ratio',
sourceField: field.name,
params:
previousColumn && previousColumn.dataType === 'number' ? previousColumn.params : undefined,
previousColumn?.dataType === 'number' &&
previousColumn.params &&
'format' in previousColumn.params
? previousColumn.params
: undefined,
};
},
toEsAggsConfig: (column, columnId) => ({
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.lnsIndexPatternDimensionEditor__filtersEditor {
width: $euiSize * 60;
}
Loading