[Lens] Settings panel redesign and separate settings per y axis #76373

stratoula · 2020-09-01T13:36:19Z

Summary

Closes #74836.
We want to give to the user the possibility to control the behavior of the left and the right axis separately. So this PR:

Redesigns the settings popover, specifically, we have settings options split on different popovers
It affects both the settings of pie/donut/treemap charts and xy axis charts
Changes the legends positions from the dropdown to button group with icons
Adds settings for the right y-axis

XY Charts

Introduce a Values popover with the missing values, popover is enabled only forarea and line charts.

Introduce a legend popover and replacing the position dropdown with icon buttons.

Introduce bottom, left and right axes popovers.

Right axis popover is enabled only when there is a right axis enabled.

Pie/Donut/Treemap Charts

Introduce the Values popover with all the corresponding settings.

Introduce the Legends popover and replacing the position dropdown with icon buttons.

Note: Icons are random atm.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

stratoula · 2020-09-02T09:08:02Z

@cchaos do you want to jump in and share your feedback?

cchaos · 2020-09-04T22:03:18Z

@stratoula I pushed you commit to get buttons looking more like groups and adding some custom icons (may port them into EUI later). The one thing I noticed is that there is still a lot of duplication. For example we could consolidate each popover contents to their own components. Especially for ones like the Legend and axis options. They can then just be customizable with the things that change from axis to axis or chart to chart but keeping the layout and form elements in one place.

stratoula · 2020-09-05T08:04:59Z

Thank you Caroline! Yes I know, this is a reason I have it on draft. Will ping you again when it will be finalised

stratoula · 2020-09-07T05:55:09Z

@elasticmachine merge upstream

stratoula · 2020-09-07T15:01:24Z

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

@@ -118,8 +119,22 @@ export const buildExpression = (
            },
          ],
          fittingFunction: [state.fittingFunction || 'None'],
-          showXAxisTitle: [state.showXAxisTitle ?? true],


I decided here instead of having 3 booleans to check the visibility of each axis title to change it to axisTitlesVisibilitySettings in order to be in line with tickLabelsVisibilitySettings and gridlinesVisibilitySettings as all of them have to do with visibility settings. If there is any objection, I am ok to revert it to the previous one.

x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx

stratoula · 2020-09-08T07:02:49Z

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

+import { FramePublicAPI } from '../types';
+import { ToolbarPopover } from '../shared_components';
+import { ToolbarButtonProps } from '../toolbar_button';
+// @ts-ignore


My dear @cchaos can we somehow avoid ts-ignore here?

It should be doable like this:

Rename axis_*.js -> axis_*.ts

Change ({ title, titleId, ...props }) -> ({ title, titleId, ...props }: : { title: string; titleId: string })

flash1293 · 2020-09-14T10:17:03Z

There is a problem with matching the axis settings to the chart.
Here I configured two metrics, both set to "right axis" - but it seems like editing the "right axis popover" is not picked up in the chart:

… scenarios

stratoula · 2020-09-14T10:29:42Z

There is a problem with matching the axis settings to the chart.
Here I configured two metrics, both set to "right axis" - but it seems like editing the "right axis popover" is not picked up in the chart:

Thanks @flash1293 for pointing this out. I missed it, I used the groupId instead and I think that now works ok

flash1293

Tested in Chrome and didn't notice any problems. This LGTM, great job!

Left a comment about the ts-ignores, once those are fixed this is good to merge from my side

flash1293 · 2020-09-14T15:43:06Z

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

+import { FramePublicAPI } from '../types';
+import { ToolbarPopover } from '../shared_components';
+import { ToolbarButtonProps } from '../toolbar_button';
+// @ts-ignore


It should be doable like this:

Rename axis_*.js -> axis_*.ts

Change ({ title, titleId, ...props }) -> ({ title, titleId, ...props }: : { title: string; titleId: string })

x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx

stratoula · 2020-09-14T15:51:37Z

Thanx @flash1293. I def agree that toolbar should be on shared folder. Let me move it on this PR 🙂

wylieconlon

LGTM, although I did find one edge case with the X overflow on the buttons: it seems like it would be nicer to wrap to a second row if possible.

stratoula · 2020-09-15T06:05:08Z

LGTM, although I did find one edge case with the X overflow on the buttons: it seems like it would be nicer to wrap to a second row if possible.

Sure @wylieconlon, it wraps on two rows now.

cchaos

Played through the different settings and everything worked great. Left a small code comment.

I also noticed that the "Axis side" button group's options in the dimension configuration doesn't match up to the sides of the graph. It seems this PR does have that knowledge and would be great to pass down to this component as well.

But other than that. 👍 LGTM!

x-pack/plugins/lens/public/shared_components/legend_settings_popover.tsx

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

stratoula · 2020-09-16T06:12:56Z

@cchaos you mean for horizontal bars instead of displaying Left to display Bottom etc, right? Now we have three axis modes (right, auto, left) and the whole logic depends on that. We could change only the label but leave the modes the same but I am not sure about that. @flash1293 what do you think?

stratoula · 2020-09-16T06:29:05Z

@elasticmachine merge upstream

kibanamachine · 2020-09-16T08:06:56Z

💚 Build Succeeded

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

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
lens	444	+8	436

page load bundle size

id	value	diff	baseline
lens	1.1MB	+27.2KB	1.1MB

History

💔 Build #74684 failed 52159b0
💚 Build #74512 succeeded 942be29
💚 Build #74407 succeeded 5b0578a
💚 Build #74361 succeeded c02337f
💚 Build #74211 succeeded 12aa5db

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

flash1293 · 2020-09-16T08:17:29Z

@stratoula The way I understand @cchaos comment is to just change the later if the chart is in "horizontal" mode and keep everything the same. I think it makes sense to change the labels because it's better aligned to what the user sees and we are doing the same already for the icons in the toolbar.

The same thing should be done for the disabled tooltip as well (should be ...when top axis is enabled...):

Also, can we show aria label as tooltip as well for the enabled popover buttons? Maybe it's just me, but I'm scared of clicking on icon buttons and often hover over them to see the title - currently they don't show one but the aria label is set correctly already.

I wouldn't consider any of this blockers as the wording is consistent, we can also separate it from the base PR as it touches a lot of stuff and create a separate followup issue.

stratoula · 2020-09-16T08:29:50Z

Thanx @flash1293, I will create a separate PR for it

…tic#76373) * wip, redsign the xy axis general settings * pie chart settings. fix tests, initial implementation * Fix Internationalization * Cleanup * remove unused translations * Add test to check that right axis is enabled * fix test * remove unecessary translation * Added icons and cleaned up some of the visuals for grouped buttons * Fix types * Axis Settings Popover Reusable Component * Legend Popover Reusable Component * Cleanup unused translations * Fix right axis behavior * Revert yLeftTitle to yTitle to avoid migration * PR fixes * identify which axis is enabled * Change the logic on enabling the y axes popovers * Adjust axis popover on horizontal bars * fix failing test and change the logic of fetching the y axis titles * Simpify the axis title logic, make the toolbar repsponsive, add TopAxisIcon * Ui Changes on legends popover * Cleanup and more unit tests * use groupId instead of index to take under consideration all possible scenarios * fix gridlines * Remove ts-ignore from icons and move toolbar button to shared components * Workspace toolbar wraps on smaller devices * Tooltip on Toolbar appears only if the button is disabled * clean up * Add missing translations * fix eslint Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…) (#77580) * wip, redsign the xy axis general settings * pie chart settings. fix tests, initial implementation * Fix Internationalization * Cleanup * remove unused translations * Add test to check that right axis is enabled * fix test * remove unecessary translation * Added icons and cleaned up some of the visuals for grouped buttons * Fix types * Axis Settings Popover Reusable Component * Legend Popover Reusable Component * Cleanup unused translations * Fix right axis behavior * Revert yLeftTitle to yTitle to avoid migration * PR fixes * identify which axis is enabled * Change the logic on enabling the y axes popovers * Adjust axis popover on horizontal bars * fix failing test and change the logic of fetching the y axis titles * Simpify the axis title logic, make the toolbar repsponsive, add TopAxisIcon * Ui Changes on legends popover * Cleanup and more unit tests * use groupId instead of index to take under consideration all possible scenarios * fix gridlines * Remove ts-ignore from icons and move toolbar button to shared components * Workspace toolbar wraps on smaller devices * Tooltip on Toolbar appears only if the button is disabled * clean up * Add missing translations * fix eslint Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (55 commits) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) ...

* master: (54 commits) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) [Ingest pipelines] Polish pipeline debugging workflow (elastic#76058) ...

* master: (76 commits) Fixing service maps API test (elastic#77586) [Grok] Fix missing error message in error toasts (elastic#77499) [Alerting] Exempt Alerts pre 7.10 from RBAC on their Action execution until updated (elastic#75563) [ML] fix type in apply_influencer_filters_action (elastic#77495) [Lens] create reusable component for filters and range aggregation (elastic#77453) fix eslint violations Collapse alert chart previews by default (elastic#77479) [ML] Fixing field caps wrapper endpoint (elastic#77546) showing service maps when filte by environment not defined (elastic#77483) [Lens] Settings panel redesign and separate settings per y axis (elastic#76373) log request body in new ES client (elastic#77150) use `navigateToUrl` to navigate to recent nav links (elastic#77446) Move core config service to `kbn/config` package (elastic#76874) [UBI] Copy license to /licenses folder (elastic#77563) Skip flaky Events Viewer Cypress test [Lens] Remove dynamic names in telemetry fields (elastic#76988) [Maps] Add DynamicStyleProperty#getMbPropertyName and DynamicStyleProperty#getMbPropertyValue (elastic#77366) [Enterprise Search] Add flag to restrict width of layout (elastic#77539) [Security Solutions][Cases - Timeline] Fix bug when adding a timeline to a case (elastic#76967) [Security Solution][Detections] Integration test for Editing a Rule (elastic#77090) ...

stratoula added 9 commits August 31, 2020 19:12

wip, redsign the xy axis general settings

f051cb4

merge with master

24ff477

pie chart settings. fix tests, initial implementation

3510ae3

Fix Internationalization

d8369f4

Cleanup

36fa43e

remove unused translations

684897c

Add test to check that right axis is enabled

f5e4676

fix test

5addfc8

remove unecessary translation

1d82ea5

stratoula changed the title ~~Lens settings redesign~~ [Lens] Settings panel redesign and separate settings per y axis Sep 2, 2020

stratoula requested a review from cchaos September 2, 2020 09:08

Added icons and cleaned up some of the visuals for grouped buttons

87b9c74

elasticmachine and others added 4 commits September 6, 2020 23:55

Merge branch 'master' into lens-settings-redesign

7b0aadc

Fix types

0733692

Axis Settings Popover Reusable Component

c00859f

Legend Popover Reusable Component

18f0928

stratoula commented Sep 7, 2020

View reviewed changes

Cleanup unused translations

161da2a

stratoula commented Sep 8, 2020

View reviewed changes

x-pack/plugins/lens/public/shared_components/toolbar_popover.tsx Show resolved Hide resolved

Fix right axis behavior

88f30d5

stratoula commented Sep 8, 2020

View reviewed changes

stratoula added Feature:Lens release_note:enhancement v8.0.0 v7.10.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Sep 8, 2020

use groupId instead of index to take under consideration all possible…

c599cb0

… scenarios

fix gridlines

9532136

flash1293 approved these changes Sep 14, 2020

View reviewed changes

Remove ts-ignore from icons and move toolbar button to shared components

12aa5db

wylieconlon approved these changes Sep 14, 2020

View reviewed changes

Workspace toolbar wraps on smaller devices

c02337f

stratoula added 3 commits September 15, 2020 12:46

resolve conficts

5b0578a

Tooltip on Toolbar appears only if the button is disabled

ae18fc3

clean up

942be29

cchaos approved these changes Sep 15, 2020

View reviewed changes

x-pack/plugins/lens/public/shared_components/legend_settings_popover.tsx Outdated Show resolved Hide resolved

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

stratoula added 2 commits September 16, 2020 09:01

Add missing translations

3a053fa

fix eslint

52159b0

Merge branch 'master' into lens-settings-redesign

c01443f

stratoula merged commit c4eb47a into elastic:master Sep 16, 2020

stratoula mentioned this pull request Sep 16, 2020

[7.x] [Lens] Settings panel redesign and separate settings per y axis (#76373) #77580

Merged

stratoula mentioned this pull request Sep 16, 2020

[Lens] Aligns the y axis position on toolbar and dimension editor on horizontal mode #77585

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Settings panel redesign and separate settings per y axis #76373

[Lens] Settings panel redesign and separate settings per y axis #76373

stratoula commented Sep 1, 2020 •

edited

Loading

stratoula commented Sep 2, 2020

cchaos commented Sep 4, 2020

stratoula commented Sep 5, 2020

stratoula commented Sep 7, 2020

stratoula Sep 7, 2020 •

edited

Loading

stratoula Sep 8, 2020

flash1293 Sep 14, 2020

flash1293 commented Sep 14, 2020

stratoula commented Sep 14, 2020

flash1293 left a comment •

edited

Loading

flash1293 Sep 14, 2020

stratoula commented Sep 14, 2020

wylieconlon left a comment

stratoula commented Sep 15, 2020

cchaos left a comment

stratoula commented Sep 16, 2020 •

edited

Loading

stratoula commented Sep 16, 2020

kibanamachine commented Sep 16, 2020

flash1293 commented Sep 16, 2020 •

edited

Loading

stratoula commented Sep 16, 2020

[Lens] Settings panel redesign and separate settings per y axis #76373

[Lens] Settings panel redesign and separate settings per y axis #76373

Conversation

stratoula commented Sep 1, 2020 • edited Loading

Summary

Checklist

stratoula commented Sep 2, 2020

cchaos commented Sep 4, 2020

stratoula commented Sep 5, 2020

stratoula commented Sep 7, 2020

stratoula Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

stratoula Sep 8, 2020

Choose a reason for hiding this comment

flash1293 Sep 14, 2020

Choose a reason for hiding this comment

flash1293 commented Sep 14, 2020

stratoula commented Sep 14, 2020

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 Sep 14, 2020

Choose a reason for hiding this comment

stratoula commented Sep 14, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

stratoula commented Sep 15, 2020

cchaos left a comment

Choose a reason for hiding this comment

stratoula commented Sep 16, 2020 • edited Loading

stratoula commented Sep 16, 2020

kibanamachine commented Sep 16, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

page load bundle size

History

flash1293 commented Sep 16, 2020 • edited Loading

stratoula commented Sep 16, 2020

stratoula commented Sep 1, 2020 •

edited

Loading

stratoula Sep 7, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading

stratoula commented Sep 16, 2020 •

edited

Loading

flash1293 commented Sep 16, 2020 •

edited

Loading