[Lens] Introduces new chart switcher #91844

dej611 · 2021-02-18T14:09:46Z

Summary

This PR introduces the new chart switcher in Lens, which is designed to better handle more charts in the future.

One chart type selected:

With multiple XY chart types (no selection but correct label on toolbar):

In case of "data loss" when changing chart type (notice the icon is on the right rather than left as in the original mockup in the issue):

Search working:

Unit and functional tests required a small amount of changes to work, but the none has been disabled and the same behaviour can be reproduced as the previous implementation.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

…itcher

elasticmachine · 2021-02-18T16:23:39Z

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

mbondyra · 2021-02-19T13:29:42Z

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

@@ -7,6 +7,17 @@

 import React, { ReactElement } from 'react';
 import { ReactWrapper } from 'enzyme';
+
+// Mock the AutoSizer inside EuiSelectable (Chart Switch) and return some dimensions > 0


Nit: Could you leave more detailed info why we have to do this (that the Autosizer behaves in particular way here)?

Agreed with Marta- can you specifically say that Jest tests render using jsdom and jsdom does not implement any sizing methods

mbondyra · 2021-02-19T13:34:48Z

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

@@ -439,6 +439,9 @@ export const visualizationTypes: VisualizationType[] = [
    label: i18n.translate('xpack.lens.xyVisualization.barLabel', {
      defaultMessage: 'Bar',
    }),
+    groupLabel: i18n.translate('xpack.lens.xyVisualization.barGroupLabel', {


nit: we repeat this copy 4 times for Bar and then another one 4 times for line or area. Maybe it's worth to assign it to variable and then read it here?

mbondyra · 2021-02-19T13:35:25Z

x-pack/plugins/lens/public/pie_visualization/constants.ts

@@ -16,18 +16,29 @@ export const CHART_NAMES = {
    label: i18n.translate('xpack.lens.pie.donutLabel', {
      defaultMessage: 'Donut',
    }),
+    groupLabel: i18n.translate('xpack.lens.pie.groupLabel', {


nit: same comment about repeating the same copy 3 times

mbondyra · 2021-02-19T13:42:06Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/chart_switch.tsx

+            }
+            return [{ key: group, label: group, isGroupLabel: true }].concat(
+              visualizations.map((v) => ({
+                'aria-label': v.label,


can we use full label for aria-label, so eg. Horizontal Bar instead of H. Bar?

Suggested change

'aria-label': v.label,

'aria-label': v.fullLabel,

title: v.fullLabel,

Can we also add title so when you hover, you see the whole title?

mbondyra

Tested in Chrome and Safari. Code LGTM with small comments. I'll approve once we'll get an OK from design team that the design changes are fine (in case if you'd have to change big chunks of code because of it)

flash1293 · 2021-02-19T14:17:42Z

Didn't test yet, but we've talked about an option to scroll to the currently active section on open (if available). Do you think that would be easy to add?

dej611 · 2021-02-19T14:25:52Z

@flash1293 I forgot about that. Will add it.
@mbondyra thanks for the review! Will integrate your feedback ASAP.

dej611 · 2021-02-19T17:15:30Z

@flash1293 the scroll to item behaviour is not exposed yet in the EuiSelectable component.
Unless strictly required I propose we could add it later on when EUI adds the method for it?

MichaelMarcialis · 2021-02-20T02:09:14Z

@dej611: This is looking great! While I was playing around with your branch this afternoon, two items caught my eye that I wanted to ask about.

It appears that you're keeping the previous chart naming convention (which included abbreviations). With the move to this more linear listing the chart types, I was hoping we could do away with the abbreviation of chart types altogether and adopt the updated names suggested in my designs, as text width is now less of a concern. Would it be possible to change them?

When using the search bar to filter chart types, it appears that the height of the menu does not decrease when the number of items is smaller than the maximum height of the menu. This ends up leaving a big blank space at the bottom of the menu. Can this be corrected to match the designs?

Regarding the swapping of sides for the alert icon tooltip, I'm good with that. There are a few other small design deviations (such as popover padding, border divider between search and selectable list, etc.), but I think changing those here would demand that we make those changes in other menus across Lens for consistency (such as the index patterns menu). If that's beyond the scope of this PR, I'm happy to put on my todo list for a later time. Thanks!

flash1293 · 2021-02-22T09:19:43Z

the scroll to item behaviour is not exposed yet in the EuiSelectable component.
Unless strictly required I propose we could add it later on when EUI adds the method for it?

I see, with virtualized scrolling it's hard to add it from the outside. In that case I guess we can live with postponing this.

…itcher

wylieconlon

Found a crash when I tested it!

wylieconlon · 2021-02-22T21:37:32Z

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

@@ -7,6 +7,17 @@

 import React, { ReactElement } from 'react';
 import { ReactWrapper } from 'enzyme';
+
+// Mock the AutoSizer inside EuiSelectable (Chart Switch) and return some dimensions > 0


Agreed with Marta- can you specifically say that Jest tests render using jsdom and jsdom does not implement any sizing methods

wylieconlon · 2021-02-22T21:48:18Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/chart_switch.tsx

+        for (const visualizationType of v.visualizationTypes) {
+          const isSearchMatch =
+            visualizationType.label.toLowerCase().includes(lowercasedSearchTerm) ||
+            visualizationType.fullLabel?.toLowerCase().includes(lowercasedSearchTerm);


So we're only looking at the i18n text for the search? I guess this makes sense...

wylieconlon · 2021-02-22T21:49:28Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/chart_switch.tsx

+          onChange={(newOptions) => {
+            const id = newOptions.find(({ checked }) => checked === 'on')!.value!;
+            commitSelection(visualizationsLookup[id].selection);
+          }}


We've had this bug before, but this appears to be a pretty big one. Lens crashes if you select the currently selected option.

wylieconlon · 2021-02-22T21:50:28Z

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

  },
  {
    id: 'line',
    icon: LensIconChartLine,
    label: i18n.translate('xpack.lens.xyVisualization.lineLabel', {
      defaultMessage: 'Line',
    }),
+    groupLabel: groupLabelForLineAndArea,


Why is Line the last chart in the "Line and area" group?

I assume this was done to match the designs, as the designs sort both the chart groups and chart items in alpha order.

dej611 · 2021-02-23T10:34:52Z

@elasticmachine merge upstream

MichaelMarcialis

Thanks for making those earlier changes, @dej611. I left a few additional wording suggestions below to better match the designs. After those wording changes are made, could you also ensure that the chart groups and the items within each group in alpha order?

Aside from that, this looks good from my perspective. Approving now so I don't hold you up further.

MichaelMarcialis · 2021-02-23T15:06:39Z

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

  },
  {
    id: 'line',
    icon: LensIconChartLine,
    label: i18n.translate('xpack.lens.xyVisualization.lineLabel', {
      defaultMessage: 'Line',
    }),
+    groupLabel: groupLabelForLineAndArea,


I assume this was done to match the designs, as the designs sort both the chart groups and chart items in alpha order.

MichaelMarcialis · 2021-02-23T15:07:52Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/chart_switch.tsx

+                    <EuiIconTip
+                      aria-label={i18n.translate('xpack.lens.chartSwitch.dataLossLabel', {
+                        defaultMessage: 'Data loss',
+                      })}
+                      type="alert"
+                      color="warning"
+                      title={i18n.translate('xpack.lens.chartSwitch.dataLossLabel', {
+                        defaultMessage: 'Data loss',
+                      })}
+                      content={i18n.translate('xpack.lens.chartSwitch.dataLossDescription', {
+                        defaultMessage:
+                          'Switching to this chart will lose some of the configuration',
+                      })}
+                      iconProps={{
+                        className: 'lnsChartSwitch__chartIcon',
+                        'data-test-subj': `lnsChartSwitchPopoverAlert_${v.id}`,
+                      }}
+                    />


Suggesting the following changes to better match the design.

Suggested change

<EuiIconTip

aria-label={i18n.translate('xpack.lens.chartSwitch.dataLossLabel', {

defaultMessage: 'Data loss',

})}

type="alert"

color="warning"

title={i18n.translate('xpack.lens.chartSwitch.dataLossLabel', {

defaultMessage: 'Data loss',

})}

content={i18n.translate('xpack.lens.chartSwitch.dataLossDescription', {

defaultMessage:

'Switching to this chart will lose some of the configuration',

})}

iconProps={{

className: 'lnsChartSwitch__chartIcon',

'data-test-subj': `lnsChartSwitchPopoverAlert_${v.id}`,

}}

/>

<EuiIconTip

aria-label={i18n.translate('xpack.lens.chartSwitch.dataLossLabel', {

defaultMessage: 'Warning',

})}

type="alert"

color="warning"

content={i18n.translate('xpack.lens.chartSwitch.dataLossDescription', {

defaultMessage:

'Selecting this chart type will result in a partial loss of currently applied configuration selections.',

})}

iconProps={{

className: 'lnsChartSwitch__chartIcon',

'data-test-subj': `lnsChartSwitchPopoverAlert_${v.id}`,

}}

/>

MichaelMarcialis · 2021-02-23T16:20:09Z

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

  },
  {
    id: 'bar_stacked',
    icon: LensIconChartBarStacked,
    label: i18n.translate('xpack.lens.xyVisualization.stackedBarLabel', {
-      defaultMessage: 'Stacked bar',
+      defaultMessage: 'Bar stacked',


To be consistent with naming and match the designs.

Suggested change

defaultMessage: 'Bar stacked',

defaultMessage: 'Bar vertical stacked',

MichaelMarcialis · 2021-02-23T16:20:52Z

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

  },
  {
    id: 'bar_percentage_stacked',
    icon: LensIconChartBarPercentage,
    label: i18n.translate('xpack.lens.xyVisualization.stackedPercentageBarLabel', {
-      defaultMessage: 'Percentage bar',
+      defaultMessage: 'Bar percentage bar',


To be consistent with naming and match the designs.

Suggested change

defaultMessage: 'Bar percentage bar',

defaultMessage: 'Bar vertical percentage',

MichaelMarcialis · 2021-02-23T16:23:04Z

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

@@ -439,6 +446,7 @@ export const visualizationTypes: VisualizationType[] = [
    label: i18n.translate('xpack.lens.xyVisualization.barLabel', {
      defaultMessage: 'Bar',


To be consistent with naming and match the designs.

Suggested change

defaultMessage: 'Bar',

defaultMessage: 'Bar vertical',

MichaelMarcialis · 2021-02-23T16:25:06Z

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

  },
  {
    id: 'area',
    icon: LensIconChartArea,
    label: i18n.translate('xpack.lens.xyVisualization.areaLabel', {
      defaultMessage: 'Area',
    }),
+    groupLabel: groupLabelForLineAndArea,
  },
  {
    id: 'area_stacked',
    icon: LensIconChartAreaStacked,
    label: i18n.translate('xpack.lens.xyVisualization.stackedAreaLabel', {
      defaultMessage: 'Stacked area',


To be consistent with naming and match the designs.

Suggested change

defaultMessage: 'Stacked area',

defaultMessage: 'Area stacked',

MichaelMarcialis · 2021-02-23T16:25:32Z

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

  },
  {
    id: 'area_stacked',
    icon: LensIconChartAreaStacked,
    label: i18n.translate('xpack.lens.xyVisualization.stackedAreaLabel', {
      defaultMessage: 'Stacked area',
    }),
+    groupLabel: groupLabelForLineAndArea,
  },
  {
    id: 'area_percentage_stacked',
    icon: LensIconChartAreaPercentage,
    label: i18n.translate('xpack.lens.xyVisualization.stackedPercentageAreaLabel', {
      defaultMessage: 'Percentage area',


To be consistent with naming and match the designs.

Suggested change

defaultMessage: 'Percentage area',

defaultMessage: 'Area percentage',

MichaelMarcialis · 2021-02-23T16:26:54Z

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

@@ -47,6 +47,9 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
      label: i18n.translate('xpack.lens.datatable.label', {
        defaultMessage: 'Data table',


To be consistent with naming and match the designs.

Suggested change

defaultMessage: 'Data table',

defaultMessage: 'Table',

flash1293

Tested in Chrome and this LGTM. I think we should kick off a flaky test runner for this - I see some flakiness potential here :)

flash1293 · 2021-02-24T16:07:12Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/chart_switch.tsx

@@ -54,6 +51,18 @@ interface Props {
  >;
 }

+interface SelectableEntry {


nit: You can replace this by type SelectableEntry = EuiSelectableOption<{ value: string }>.

There's one small issue with the type of the array returned in 272 because concat is not playing nice, but you can cast it using as SelectableEntry[]

Yes, that was the main reason to redeclare it from scratch. I'll see if it's possible to fix it without force casting it. In case it's not possible, I'll force cast 😅

dej611 · 2021-02-25T13:20:57Z

@elasticmachine merge upstream

wylieconlon

Looks good to me!

x-pack/plugins/lens/public/xy_visualization/visualization.tsx

Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>

dej611 · 2021-03-01T09:01:21Z

@elasticmachine merge upstream

kibanamachine · 2021-03-01T16:24:00Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	918.2KB	920.8KB	+2.6KB

History

💔 Build #109812 failed 99d5be4
💚 Build #109380 succeeded 41e21b0
💚 Build #109134 succeeded 71ec4be
💚 Build #108993 succeeded 208cdfb
💔 Build #108836 failed 09b9ff2

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

Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (42 commits) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (elastic#92932) ...

… playwright-ftr-e2e * 'playwright-ftr-e2e' of github.com:shahzad31/kibana: (38 commits) [chore] Enable core's eslint rule: `@ts-expect-error` (elastic#93086) [Lens] Introduces new chart switcher (elastic#91844) [Lens] fix selection when dragging (elastic#93034) Converts usage collection README to .mdx (elastic#92982) Fix expanding document when using saved search data grid (elastic#92999) [SECURITY SOLUTIONS] Bug case connector (elastic#93104) [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025) [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942) [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139) [DOCS] Fixes links for machine learning alerts (elastic#92744) [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748) [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010) [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095) [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667) Bump ems landing page to 7.12 (elastic#93065) [App Search] Implement various Relevance Tuning states and form actions (elastic#92644) [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760) [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092) Hide instances latency distribution chart (elastic#92869) [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087) ...

Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

dej611 added 5 commits February 17, 2021 15:14

✨ First implementation completed

5fd3f23

✅ fix unit tests

d28a394

✅ Fix functional tests

78b8218

Merge remote-tracking branch 'upstream/master' into lens/new-chart-sw…

b9b28aa

…itcher

🏷️ Fix type checks

89bcb5d

dej611 added v8.0.0 Feature:Lens v7.13.0 labels Feb 18, 2021

dej611 requested a review from MichaelMarcialis February 18, 2021 16:23

dej611 added release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Feb 18, 2021

dej611 marked this pull request as ready for review February 18, 2021 16:23

dej611 requested a review from a team February 18, 2021 16:23

dej611 requested a review from a team as a code owner February 18, 2021 16:23

dej611 mentioned this pull request Feb 18, 2021

[EuiSelectable] Missing or partial documentation and types elastic/eui#4545

Open

mbondyra reviewed Feb 19, 2021

View reviewed changes

dej611 added 3 commits February 19, 2021 17:11

💡 Add more explanation for the mock

5a8195e

💬 Use full label for aria label

0340e4e

♻️ Refactored group labels

79a6731

Merge remote-tracking branch 'upstream/master' into lens/new-chart-sw…

2cc0032

…itcher

dej611 added 3 commits February 22, 2021 12:10

💄 Adapt list height based on result size

7abba6c

♻️ Write numbers as constants

0bd7e7c

💬 Revisit vis labels as suggested

0a060ea

wylieconlon suggested changes Feb 22, 2021

View reviewed changes

Merge branch 'master' into lens/new-chart-switcher

40f1d19

MichaelMarcialis approved these changes Feb 23, 2021

View reviewed changes

dej611 added 2 commits February 23, 2021 19:02

👌 Integrated feedback

28035d8

🐛 Fix some more copy and failing tests

09b9ff2

dej611 requested a review from wylieconlon February 23, 2021 18:20

🐛 Fix i18n check

208cdfb

flash1293 approved these changes Feb 24, 2021

View reviewed changes

🏷️ Refactor type

71ec4be

dej611 requested a review from mbondyra February 25, 2021 08:50

Merge branch 'master' into lens/new-chart-switcher

41e21b0

wylieconlon approved these changes Feb 25, 2021

View reviewed changes

x-pack/plugins/lens/public/xy_visualization/visualization.tsx Outdated Show resolved Hide resolved

Update x-pack/plugins/lens/public/xy_visualization/visualization.tsx

dc262c3

Co-authored-by: Wylie Conlon <wylieconlon@gmail.com>

kibanamachine and others added 2 commits March 1, 2021 04:01

Merge branch 'master' into lens/new-chart-switcher

99d5be4

🐛 Fix unit test

9986e6e

dej611 merged commit 03cc5cc into elastic:master Mar 2, 2021

dej611 deleted the lens/new-chart-switcher branch March 2, 2021 10:25

dej611 mentioned this pull request Mar 2, 2021

[7.x] [Lens] Introduces new chart switcher (#91844) #93190

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Mar 2, 2021

[Lens] Introduces new chart switcher (elastic#91844)

2f52f83

Co-authored-by: Wylie Conlon <wylieconlon@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

	'aria-label': v.label,
	'aria-label': v.fullLabel,
	title: v.fullLabel,

	defaultMessage: 'Bar stacked',
	defaultMessage: 'Bar vertical stacked',

	defaultMessage: 'Bar percentage bar',
	defaultMessage: 'Bar vertical percentage',

		@@ -439,6 +446,7 @@ export const visualizationTypes: VisualizationType[] = [
		label: i18n.translate('xpack.lens.xyVisualization.barLabel', {
		defaultMessage: 'Bar',

	defaultMessage: 'Stacked area',
	defaultMessage: 'Area stacked',

	defaultMessage: 'Percentage area',
	defaultMessage: 'Area percentage',

		@@ -47,6 +47,9 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
		label: i18n.translate('xpack.lens.datatable.label', {
		defaultMessage: 'Data table',

[Lens] Introduces new chart switcher #91844

[Lens] Introduces new chart switcher #91844

Conversation

dej611 commented Feb 18, 2021

Summary

Checklist

elasticmachine commented Feb 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbondyra Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

mbondyra left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 commented Feb 19, 2021

dej611 commented Feb 19, 2021

dej611 commented Feb 19, 2021

MichaelMarcialis commented Feb 20, 2021

flash1293 commented Feb 22, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Feb 23, 2021

MichaelMarcialis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dej611 commented Feb 25, 2021

wylieconlon left a comment

Choose a reason for hiding this comment

dej611 commented Mar 1, 2021

kibanamachine commented Mar 1, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

mbondyra Feb 19, 2021 •

edited

Loading

mbondyra left a comment •

edited

Loading