[Lens] Fieldless operations #78080

wylieconlon · 2020-09-21T20:34:33Z

This PR begins categorizing operations in Lens by the type of input that they take, switching from the previous state of field-based operations to having two types. In the future, we will add a third type to support the calculations internal API.

Screenshots

Technical summary of changes

There are two types of changes here: changes that affect the UI for building dimensions, and then a bunch of Typescript changes associated.

Changes that affect the UI:

There are very few changes here:

Operation definitions are required to provide a value of input: 'none' or input: 'field', which is read by the dimension editor.
The dimension editor will show the field selector unless looking at an operation with input: 'none'
Users can now click the "current" operation in the list, which will restore the state they were previously looking at.

Typescript changes:

In addition to the required input property, we now use a discriminated union type on the operation definitions. This lets us define use-case-specific functions for each operation input, while maintaining some shared logic on all the operations.

Closes #75231

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…ations

elasticmachine · 2020-09-22T21:35:36Z

Pinging @elastic/kibana-app (Team:KibanaApp)

wylieconlon · 2020-09-22T21:37:47Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx

+        selectedOperationDefinition?.input === 'field' ||
+        (incompatibleSelectedOperationType &&
+          operationDefinitionMap[incompatibleSelectedOperationType].input === 'field') ? (
+          <EuiFormRow


For reviewers: this is an indentation change, not a functional change inside this code.

wylieconlon · 2020-09-22T21:40:15Z

x-pack/test/functional/apps/lens/smokescreen.ts

      expect(await find.allByCssSelector('.echLegendItem')).to.have.length(3);
    });
+
    it('should create an xy visualization with filters aggregation', async () => {


In my opinion this function test is covering the simplest case, and I think I've covered the other cases in jest-based tests

cchaos

Screenshots LGTM, no SASS changes 👍

flash1293

Didn't get to a thorough review yet (will do that tomorrow), but I couldn't find any problems with the behavior. The biggest concern I have is to remove the type param from some of the operation functions and the param editor - these casts don't look necessary but maybe I'm missing something.

I started these changes:

interface BaseOperationDefinitionProps<C extends BaseIndexPatternColumn> {
  type: C['operationType'];
  /**
   * The priority of the operation. If multiple operations are possible in
   * a given scenario (e.g. the user dragged a field into the workspace),
   * the operation with the highest priority is picked.
   */
  priority?: number;
  /**
   * The name of the operation shown to the user (e.g. in the popover editor).
   * Should be i18n-ified.
   */
  displayName: string;
  /**
   * This function is called if another column in the same layer changed or got removed.
   * Can be used to update references to other columns (e.g. for sorting).
   * Based on the current column and the other updated columns, this function has to
   * return an updated column. If not implemented, the `id` function is used instead.
   */
  onOtherColumnChanged?: (
    currentColumn: C,
    columns: Partial<Record<string, IndexPatternColumn>>
  ) => C;
  /**
   * React component for operation specific settings shown in the popover editor
   */
  paramEditor?: React.ComponentType<ParamEditorProps<C>>;
  /**
   * Function turning a column into an agg config passed to the `esaggs` function
   * together with the agg configs returned from other columns.
   */
  toEsAggsConfig: (column: C, columnId: string, indexPattern: IndexPattern) => unknown;
  /**

and could remove the casts without running into type errors.

flash1293 · 2020-09-23T15:18:35Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/date_histogram.tsx

  },
  isTransferable: (column, newIndexPattern) => {
-    const newField = newIndexPattern.fields.find((field) => field.name === column.sourceField);
+    const c = column as DateHistogramIndexPatternColumn;


It seems a bit ugly we need those casts everywhere. What was the reason for typing column by the column type of the current operation?

…ations

wylieconlon · 2020-09-23T16:03:27Z

@flash1293 We definitely needed type casts in a few places, but I agree that it was not great to push them into each operation definition. I think I have a solution to push the casting into a shared place instead of each child.

flash1293 · 2020-09-23T16:17:33Z

@wylieconlon Sounds great, thanks. I will give it a thorough look tomorrow morning.

flash1293 · 2020-09-24T08:50:03Z

@elasticmachine merge upstream

flash1293

Tested in Chrome and Firefox and didn't notice any issues, LGTM.

I think the type setup is fine that way, I played around a bit but didn't find a better solution.

The only thing I noticed is a small inconsistency in the wording of the error message on switching from filters to something else:

There is no field selected so it's not really a "different field", it's just a field

…ations

wylieconlon · 2020-09-24T20:56:46Z

@flash1293 Thanks for the suggestion, I've added a new warning which was "To use this function, select a field." to handle this case.

mbondyra · 2020-09-28T11:31:16Z

@elasticmachine merge upstream

mbondyra

Tested on Safari, all works as expected. Code LGTM.

kibanamachine · 2020-09-28T13:00:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bc0563c

Metrics [docs]

page load bundle size

id	value	diff	baseline
lens	1.0MB	+1.5KB	1.0MB

History

💔 Build #77137 failed 0d81905
💔 Build #77131 failed 454afc2
💚 Build #76837 succeeded 207af91
💔 Build #76586 failed 90f26d4
💚 Build #76347 succeeded a1a05eb

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

* [Lens] Fieldless operations * Overhaul types * Fix invalid state and add tests * Fix types * Small cleanup * Add additional error message * Reset field selector to empty state when invalid Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

* master: Fix APM lodash imports (elastic#78438) Add deprecated message to tile_map and region_map visualizations. (elastic#77683) Fix Lens smokescreen flaky tests (elastic#78566) updated discover with alt text (elastic#77660) Fix types (elastic#78619) Update tutorial-visualizing.asciidoc (elastic#76977) Update tutorial-discovering.asciidoc (elastic#76976) [Search] Error notification alignment (elastic#77788) Update tutorial-define-index.asciidoc (elastic#76975) [Lens] Fieldless operations (elastic#78080) [Usage Collection] [schema] Explicit "array" definition (elastic#78141) Update tutorial-define-index.asciidoc (elastic#76973) Fix --no-basepath references in doc (elastic#78570) Move StubIndexPattern to data plugin and convert to TS. (elastic#78518) Index pattern class - remove unused methods (elastic#78538) [Security Solution] [ALL] Eliminates all console.error and console.warn from Jest output (elastic#78523) [Actions] avoids setting a default dedupKey on PagerDuty (elastic#77773) First stab at developer-focussed saved objects docs (elastic#71430) remove unnecessary config validations (elastic#78527)

wylieconlon added 2 commits September 21, 2020 16:33

[Lens] Fieldless operations

381b61a

Overhaul types

6e41b8b

wylieconlon added Feature:Lens Project:LensDefault labels Sep 21, 2020

wylieconlon added 2 commits September 22, 2020 13:20

Merge remote-tracking branch 'origin/master' into lens/fieldless-oper…

87b791c

…ations

Fix invalid state and add tests

a1a05eb

wylieconlon added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Sep 22, 2020

wylieconlon requested review from cchaos, dej611, flash1293 and mbondyra September 22, 2020 21:35

wylieconlon marked this pull request as ready for review September 22, 2020 21:35

wylieconlon requested a review from a team September 22, 2020 21:35

wylieconlon commented Sep 22, 2020

View reviewed changes

cchaos approved these changes Sep 22, 2020

View reviewed changes

flash1293 reviewed Sep 23, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into lens/fieldless-oper…

2f438b5

…ations

wylieconlon added 2 commits September 23, 2020 12:23

Fix types

90f26d4

Small cleanup

cb02b8c

Merge branch 'master' into lens/fieldless-operations

207af91

flash1293 approved these changes Sep 24, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into lens/fieldless-oper…

7d59c70

…ations

Add additional error message

454afc2

Reset field selector to empty state when invalid

0d81905

Merge branch 'master' into lens/fieldless-operations

bc0563c

mbondyra approved these changes Sep 28, 2020

View reviewed changes

wylieconlon merged commit 0ebaf92 into elastic:master Sep 28, 2020

wylieconlon deleted the lens/fieldless-operations branch September 28, 2020 15:22

wylieconlon mentioned this pull request Sep 28, 2020

[7.x] [Lens] Fieldless operations (#78080) #78620

Merged

[Lens] Fieldless operations #78080

[Lens] Fieldless operations #78080

Uh oh!

Conversation

wylieconlon commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Technical summary of changes

Checklist

Uh oh!

elasticmachine commented Sep 22, 2020

Uh oh!

wylieconlon Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

wylieconlon Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

wylieconlon commented Sep 23, 2020

Uh oh!

flash1293 commented Sep 23, 2020

Uh oh!

flash1293 commented Sep 24, 2020

Uh oh!

flash1293 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wylieconlon commented Sep 24, 2020

Uh oh!

mbondyra commented Sep 28, 2020

Uh oh!

mbondyra left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Sep 28, 2020

💚 Build Succeeded

Metrics [docs]

page load bundle size

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wylieconlon commented Sep 21, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading