[Lens] Simplify state management from visualization by wylieconlon · Pull Request #58279 · elastic/kibana

wylieconlon · 2020-02-21T20:58:21Z

Release note

Lens now shows a warning when you have partially configured a visualization, such as a bar chart with only an X axis

Developer notes

This change is a combination of:

Making the right side configuration panel consistent for all visualizations
Drag & drop behavior is not just for one datasource- any datasource can now use this
Visualizations now have a way of knowing when they have empty columns, which was not possible before

…guration

elasticmachine · 2020-02-21T20:58:23Z

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

…guration

wylieconlon · 2020-03-09T16:05:54Z

x-pack/legacy/plugins/lens/public/types.ts

-
  renderLayerContextMenu?: (domElement: Element, props: VisualizationLayerConfigProps<T>) => void;

+  getLayerOptions: (props: VisualizationLayerConfigProps<T>) => VisualizationLayerConfigResult;


For reviewers: This line of code is one of the key changes. Visualizations are no longer allowed to have a renderDimensionPanel method, and instead provide an object that defines what the right side panel does https://github.com/elastic/kibana/pull/58279/files#diff-6ae9ccca58dffda903f0e2042bcf455bR258

wylieconlon · 2020-03-09T16:08:16Z

x-pack/legacy/plugins/lens/public/types.ts

+  renderDimensionTrigger: (domElement: Element, props: DatasourceDimensionTriggerProps<T>) => void;
+  renderDimensionEditor: (domElement: Element, props: DatasourceDimensionEditorProps<T>) => void;
+  canHandleDrop: (props: DatasourceDimensionDropProps<T>) => boolean;
+  onDrop: (props: DatasourceDimensionDropHandlerProps<T>) => boolean;


For reviewers: one of the best changes I've done here is to make drag & drop behavior and popover behavior into a feature of the Lens editor, not a feature of the data source. This will let us add new datasources.

wylieconlon · 2020-03-09T16:09:45Z

x-pack/legacy/plugins/lens/public/indexpattern_datasource/dimension_panel/popover_editor.tsx

-          <EuiLink
-            className="lnsPopoverEditor__link"
-            onClick={() => {
-              setPopoverOpen(!isPopoverOpen);


For reviewers: Notice that we have removed EuiPopover from the dimension editor. This logic is moved into the Editor Frame.

wylieconlon · 2020-03-09T16:10:36Z

x-pack/legacy/plugins/lens/public/indexpattern_datasource/dimension_panel/popover_editor.tsx

-                id="xpack.lens.configure.emptyConfig"
-                defaultMessage="Drop a field here"
-              />
-            </EuiButtonEmpty>


For reviewers: The 'Add a configuration' button is no longer owned by the datasource.

wylieconlon · 2020-03-09T16:11:37Z

x-pack/legacy/plugins/lens/public/indexpattern_datasource/indexpattern.tsx

-      layerId,
-      dateRange,
-    }: PublicAPIProps<IndexPatternPrivateState>) {
+    renderDimensionTrigger: (


For reviewers: renderDimensionTrigger is only rendering the clickable link. It has almost no behavior any more.

wylieconlon · 2020-03-09T16:15:36Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/config_panel_wrapper.tsx

+        <EuiSpacer size="s" />
+
+        {dimensions.map((dimension, index) => {
+          const newId = generateId();


For reviewers: This call to generateId() is important. I will write some other comments here explaining why.

wylieconlon · 2020-03-09T16:16:31Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/config_panel_wrapper.tsx

+                          })
+                        );
+                      }
+                    }}


For reviewers: Notice that the generated ID is used multiple times in the drop interaction. This guarantees that both the datasource and visualization agree on the ID.

wylieconlon · 2020-03-09T16:17:22Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/config_panel_wrapper.tsx

+                  el.blur();
+                }
+
+                onRemove();


For reviewers: This is an indentation change, not a behavior change.

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/config_panel_wrapper.tsx

wylieconlon · 2020-03-09T16:21:05Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/config_panel_wrapper.tsx

+          {datasourcePublicAPI && (
+            <EuiFlexItem className="eui-textTruncate">
+              <NativeRenderer
+                render={datasourcePublicAPI.renderLayerPanel}


For reviewers: This is an indentation change. renderLayerPanel currently renders the name of the index pattern on each layer.

x-pack/legacy/plugins/lens/public/_config_panel.scss

x-pack/legacy/plugins/lens/public/indexpattern_datasource/dimension_panel/_dimension_panel.scss

cchaos

Something really funky is happening to the config panel.

The sizes of the dimension panels are wrong

and when there's a configuration, it's gets even funkier

Also I found a rogue )

x-pack/legacy/plugins/lens/public/_config_panel.scss

cchaos

The styles seem to be back in place now. I do think that ConfigPanelWrapper could be broken down into smaller components since the file is quite large. And same with the PopoverEditor which I got quite lost in trying ensure class name updates and consistency. But testing in the UI, it all seems to be back where it was so 🤷‍♀ .

cchaos · 2020-03-12T18:44:49Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/_config_panel_wrapper.scss

+
+.lnsConfigPanel__addLayerBtn {
+  color: transparentize($euiColorMediumShade, .3);
+  // sass-lint:disable-block no-important


Suggested change

// sass-lint:disable-block no-important

// Remove EuiButton's default shadow to make button more subtle

// sass-lint:disable-block no-important

…guration

mbondyra · 2020-03-13T08:58:02Z

@wylieconlon I think when it comes to the behaviour, I have nothing else to add. I checked on master and I think that the bug 1 behaves better there (or I actually am not seeing the bug there), the rest is the same.

timroes · 2020-03-13T09:05:45Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.test.tsx

+        datasourceId: 'testDatasource',
        renderLayerPanel: jest.fn(),
-        renderDimensionPanel: jest.fn(),
+        // renderDimensionPanel: jest.fn(),


Reminder for commented out code.

mbondyra · 2020-03-13T09:11:34Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/config_panel_wrapper.tsx

+                // activeElement does not have blur so, we need to do some casting + safeguards.
+                const el = (document.activeElement as unknown) as { blur: () => void };
+
+                if (el && el.blur) {


nitpick, we could probably do el?.blur

timroes · 2020-03-13T10:38:34Z

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

+        props.togglePopover();
+      }}
+      data-test-subj="lns-dimensionTrigger"
+      aria-label={i18n.translate('xpack.lens.configure.editConfig', {


We're trying not to have two i18n.translate calls with the same id in it, since it's prone to introduce errors where default messages vary later on. Could we please just call this once and store it within a variable that is used here.

It's not possible to have two i18n IDs with different default messages. It's checked automatically. How important is this?

mbondyra · 2020-03-13T10:49:47Z

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

+  const layerId = props.layerId;
+  const currentIndexPattern =
+    props.state.indexPatterns[props.state.layers[layerId]?.indexPatternId];
+  const operationFieldSupportMatrix = getOperationFieldSupportMatrix(props);


Shouldn't it have to be memoized just like it was before? 🤔 I noticed you memoized the whole component but as the function output depends on currentIndexPattern and filterOperations, maybe it's worth to do it too?

I would like it to be memoized, but I couldn't find a way to do it safely. Each dimension needs to be memoized separately.

timroes · 2020-03-13T11:01:46Z

x-pack/legacy/plugins/lens/public/editor_frame_service/editor_frame/config_panel_wrapper.tsx

  const datasourcePublicAPI = framePublicAPI.datasourceLayers[layerId];
-  const layerConfigProps = {
+  if (!datasourcePublicAPI) {
+    return <></>;


Could we return null instead here?

mbondyra · 2020-03-13T11:27:22Z

x-pack/legacy/plugins/lens/public/datatable_visualization/visualization.tsx

+    };
+  },
+
+  setDimension({ layerId, columnId, prevState }) {


nit: for readability we could change the order with matching removeDimension params order

Suggested change

setDimension({ layerId, columnId, prevState }) {

setDimension({ prevState, columnId, layerId }) {

…guration

kibanamachine · 2020-03-16T21:07:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5a64382

History

💚 Build #32630 succeeded d3077cd
💔 Build #32604 failed 1160987
💔 Build #32240 failed 8eb8fac
💔 Build #31597 failed e8b63fa
💔 Build #31547 failed 0e22ae5

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

mbondyra · 2020-03-17T09:33:05Z

Re-tested locally on Chrome, LGTM.

timroes

Checked code, LGTM

* [Lens] Declarative right panel * Fix memoized operations * Add error checking * Fix dimension panel tests * More updates * Fix all editor frame tests * Fix jest tests * Fix bug with removing dimension * Update tests * Fix frame tests * Fix all tests I could find * Remove debugger * Style config panels * Update i18n * Fix dashboard test * Fix bug when switching index patterns

* master: [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358)

* master: (51 commits) do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) Fix import to timefilter from in TSVB (elastic#60296) [NP] Get rid of usage redirectWhenMissing service (elastic#59777) ...

* alerting/view-in-app: (53 commits) fixed typo handle optional alerting plugin do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) ...

wylieconlon added 3 commits February 20, 2020 16:12

[Lens] Declarative right panel

7b6aa3c

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

783875f

…guration

Fix memoized operations

7ab5ca1

wylieconlon added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// Feature:Lens labels Feb 21, 2020

wylieconlon added 8 commits March 3, 2020 14:27

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

4041c09

…guration

Add error checking

c23c3f9

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

449e3a8

…guration

Fix dimension panel tests

92a8bea

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

c1a44fb

…guration

More updates

9178b13

Fix all editor frame tests

0e22ae5

Fix jest tests

e8b63fa

wylieconlon marked this pull request as ready for review March 6, 2020 23:31

wylieconlon requested review from a team, mbondyra and timroes March 6, 2020 23:31

wylieconlon added release_note:enhancement v7.7.0 v8.0.0 labels Mar 6, 2020

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

cdc488f

…guration

wylieconlon commented Mar 9, 2020

View reviewed changes

wylieconlon mentioned this pull request Mar 9, 2020

[Lens] Dropping numerical field into dimension wrongly select Minimum aggregation #58331

Closed

Fix bug with removing dimension

ef403a6

wylieconlon requested a review from a team as a code owner March 9, 2020 21:04

cchaos reviewed Mar 9, 2020

View reviewed changes

x-pack/legacy/plugins/lens/public/_config_panel.scss Show resolved Hide resolved

wylieconlon commented Mar 9, 2020

View reviewed changes

x-pack/legacy/plugins/lens/public/indexpattern_datasource/dimension_panel/_dimension_panel.scss Show resolved Hide resolved

cchaos suggested changes Mar 10, 2020

View reviewed changes

x-pack/legacy/plugins/lens/public/_config_panel.scss Show resolved Hide resolved

Update tests

a2d5e14

wylieconlon mentioned this pull request Mar 12, 2020

[Lens] Resetting a layer should clear suggestions #60045

Closed

cchaos approved these changes Mar 12, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

4713e63

…guration

timroes reviewed Mar 13, 2020

View reviewed changes

mbondyra reviewed Mar 13, 2020

View reviewed changes

timroes reviewed Mar 13, 2020

View reviewed changes

mbondyra reviewed Mar 13, 2020

View reviewed changes

timroes reviewed Mar 13, 2020

View reviewed changes

mbondyra reviewed Mar 13, 2020

View reviewed changes

wylieconlon added 3 commits March 13, 2020 13:55

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

83a438c

…guration

Fix bug when switching index patterns

07531df

Merge remote-tracking branch 'origin/master' into lens/simplify-confi…

5a64382

…guration

wylieconlon requested review from mbondyra and timroes March 16, 2020 19:36

mbondyra approved these changes Mar 17, 2020

View reviewed changes

timroes approved these changes Mar 17, 2020

View reviewed changes

wylieconlon merged commit caed9ba into elastic:master Mar 17, 2020

wylieconlon deleted the lens/simplify-configuration branch March 17, 2020 13:57

wylieconlon mentioned this pull request Mar 17, 2020

[7.x] [Lens] Simplify state management from visualization (#58279) #60375

Merged

wylieconlon mentioned this pull request Mar 17, 2020

[Lens] Removing a dimension in the table crashes the app #60404

Closed

This was referenced Apr 6, 2020

[Lens] Fix empty dimensions after upgrading from pre-7.7 #62707

Closed

[Lens] [7.7] Migration for 7.7 #62877

Merged


		renderLayerContextMenu?: (domElement: Element, props: VisualizationLayerConfigProps<T>) => void;

		getLayerOptions: (props: VisualizationLayerConfigProps<T>) => VisualizationLayerConfigResult;

	// sass-lint:disable-block no-important
	// Remove EuiButton's default shadow to make button more subtle
	// sass-lint:disable-block no-important

	setDimension({ layerId, columnId, prevState }) {
	setDimension({ prevState, columnId, layerId }) {

Conversation

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

Uh oh!

Release note

Developer notes

Uh oh!

elasticmachine commented Feb 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbondyra commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbondyra Mar 13, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbondyra Mar 13, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 16, 2020

💚 Build Succeeded

History

Uh oh!

mbondyra commented Mar 17, 2020

Uh oh!

timroes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

wylieconlon commented Feb 21, 2020 •

edited

Loading

mbondyra commented Mar 13, 2020 •

edited

Loading

mbondyra Mar 13, 2020 •

edited

Loading

mbondyra Mar 13, 2020 •

edited

Loading