[Lens] Allow modifying curve type for line/area series charts by shahzad31 · Pull Request #94675 · elastic/kibana

shahzad31 · 2021-03-16T10:01:23Z

Summary

Default will be CurveType.Linear

elasticmachine · 2021-03-16T10:02:19Z

Pinging @elastic/uptime (Team:uptime)

flash1293 · 2021-03-16T13:12:22Z

Thanks for looking into this! As this feature entered the roadmap on short notice, there are some UI/UX questions we haven't answered. Let's try to do so on this PR.

(probably not relevant, but I at least want to bring it up) No option is always the best - should we just always curve lines? As far as I understand this would satisfy the Obs use case. I'm not sold on it though because it makes it less fitting for exact data.
If we want to offer an option to the user here (basically switching between an exact chart and a chart highlighting the overall trend instead of the exact data points), do we want to offer a list of different curving algos? I'm no expert on this topic (but I guess our users aren't either), and I would expect a simple switch here - curved vs. not curved.
Do we really need a separate popover for this? I would vote for making this part of the "Values" popover which already houses the value labels setting for bar charts and the missing values handling. I guess in this case we would have to rename the menu (what about "Visual options" and the brush icon)?

cc @dej611 @mbondyra @MichaelMarcialis @markov00

markov00 · 2021-03-16T14:21:30Z

(probably not relevant, but I at least want to bring it up) No option is always the best - should we just always curve lines? As far as I understand this would satisfy the Obs use case. I'm not sold on it though because it makes it less fitting for exact data.

I have a different view here: I rather prefer a valid default and the option to change it, than having a valid default for most cases but no option to change it.
There is always the case where the analyst, the designer, or the final end-user wants to tweak a particular part of the visualization, if we have that ability why completely removing that from the user's hands? I think we should provide always a good set of defaults for beginner users, but we should provide enough flexibility for power users. Unfortunately, there isn't a real standard in data visualization but conventions applied to cases.

If we want to offer an option to the user here (basically switching between an exact chart and a chart highlighting the overall trend instead of the exact data points), do we want to offer a list of different curving algos? I'm no expert on this topic (but I guess our users aren't either), and I would expect a simple switch here - curved vs. not curved.

The monotonic along X is usually the curve that best represents a trend along the x-axis (like most of our use cases), without exaggerating the turns.

I think it's fine for a first version to expose only the monotone x curve type, and I think is fine to hide that under a "curved" label instead of the function name, just remember to use the function name when saving it so internally we know exactly what function was used.

A nice addition can be also the stepped function, we have 3 different functions on charts for this type of interpolation

Do we really need a separate popover for this? I would vote for making this part of the "Values" popover which already houses the value labels setting for bar charts and the missing values handling. I guess in this case we would have to rename the menu (what about "Visual options" and the brush icon)?

I've the same opinion here, we should start probably populate a single menu with visual options rather then having a button for each of them

flash1293 · 2021-03-16T14:32:36Z

Thanks @markov00 , that's great input!

shahzad31 · 2021-03-17T08:34:43Z

I also like the brush icon idea and keeping it in the same popover. I will go ahead and make the change, if there are no objections.

shahzad31 · 2021-03-22T08:01:11Z

Update: @flash1293 Switched to curved/non-surved option and consolidated in a single popover

elasticmachine · 2021-03-22T13:36:16Z

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

flash1293 · 2021-03-22T13:36:29Z

Seems like some tests have to be adjusted, but looks pretty good.

MichaelMarcialis

I'm liking this direction that you've all proceeded with. Left a few comments/questions below.

MichaelMarcialis · 2021-03-22T18:07:26Z

x-pack/plugins/lens/public/shared_components/visual_options_popover/line_curve_option.tsx

+      label={
+        <>
+          {i18n.translate('xpack.lens.xyChart.curveStyleLabel', {
+            defaultMessage: 'Line style',


For consistency with how we've previously presented most switches across Lens, can you change the EuiFormRow label to something like Curve lines (and subsequently hide the label on the EuiSwitch component via the showLabel prop)?

MichaelMarcialis · 2021-03-22T18:10:01Z

x-pack/plugins/lens/public/shared_components/visual_options_popover/line_curve_option.tsx

+          <EuiIconTip
+            color="subdued"
+            content={i18n.translate('xpack.lens.xyChart.curveStyleLabelHelpText', {
+              defaultMessage: `By default, Lines are not curved.`,
+            })}
+            iconProps={{
+              className: 'eui-alignTop',
+            }}
+            position="top"
+            size="s"
+            type="questionInCircle"
+          />


Minor nitpick: I'd personally be OK with removing this tooltip altogether, as I imagine the default will be evident to users during the creation process.

MichaelMarcialis · 2021-03-22T18:14:47Z

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

+  return (
+    <ToolbarPopover
+      title={i18n.translate('xpack.lens.shared.curveLabel', {
+        defaultMessage: 'Visual options',


Any thoughts on changing Visual options to Appearance options?

Honestly to me seems like Visual and Appearance both are loaded words, but i am not a native speaker :D
It could be just simply Style options? then again style could be more of a css thing. i am 50/50 on Visual and Appearance. may be slightly in favor of Visual.

No opinion on this, previously Gail would do a pass over all newly introduced wording before the release. I think we can go either way on this PR and let her check before we push it out.

markov00 · 2021-03-23T09:10:23Z

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

+      })}
+      type="visualOptions"
+      groupPosition="right"
+      buttonDataTestSubj="lnsLegendButton"


Is this probably a wrong copy and paste or have I miss something? it was lnsValuesButton in the previous code

yes, i am cleaning the code, this should get cleaned as well, with tests alongside.

shahzad31 · 2021-03-23T12:48:40Z

Does anyone have any opinion about keeping default option as Curved? i feel like that aligns with providing user with sensible defaults kibana wide. I feel like providing curved lines is a sensible default.

shahzad31 · 2021-03-24T09:00:23Z

@wylieconlon that sounds like a good idea, but i guess we can do that as an enhancement. I think curved/non curved is a good start.

shahzad31 · 2021-03-24T09:01:28Z

@flash1293 this is ready as far tests etc goes.

flash1293

Looks almost good to me, left two minor comments.

I agree we can add stepped in a follow-up, this is definitely a valuable increment in itself.

Edit:

I noticed one issue: On switching from a curved line chart to an area chart, the curve option gets lost. Make sure to pass the setting along in the suggestions logic:

kibana/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts

Line 237 in 16d86b0

changeType,

flash1293 · 2021-03-24T10:22:15Z

x-pack/plugins/lens/public/shared_components/visual_options_popover/missing_values_option.tsx

+
+  return (
+    <>
+      {isValueLabelsEnabled ? (


nit: You can use isValueLabelsEnabled && syntax here to get rid of the : null

flash1293 · 2021-03-24T10:25:02Z

x-pack/plugins/lens/public/shared_components/visual_options_popover/line_curve_option.test.tsx

@@ -0,0 +1,44 @@
+/*


I might miss something, but the visual options are only used for the xy visualization, right? In that case they should be located in the xy_visualization folder as well (shared_components is meant for things used in multiple chart types)

flash1293 · 2021-03-24T10:36:33Z

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

+    curveType: {
+      types: ['number'],
+      help: i18n.translate('xpack.lens.xyChart.curveType.help', {
+        defaultMessage: 'Define how curve type is rendered for a line chart',


Can we pass the enum here instead of the number (and resolve it in the component using CurveType[arg])? That seems easier to follow (and I'm somehow scared of the numbers changing and nobody notices)

i am not sure how enum will work here. can you give an example?

flash1293 · 2021-03-25T12:36:37Z

It doesn't look like the comment I added via edit was addressed: #94675 (review)

I noticed one issue: On switching from a curved line chart to an area chart, the curve option gets lost. Make sure to pass the setting along in the suggestions logic:

kibana/x-pack/plugins/lens/public/xy_visualization/xy_suggestions.ts

Line 237 in 16d86b0

changeType,

Switching between area/line charts and adding dimensions should preserve the "curve" option

flash1293

See my comment

wylieconlon

While testing this I found a bug: using the Area Percentage chart type with a histogram shows this set of messages:

It seems like we do not allow fitting functions for Area Percentage, but we do allow curving now. The message is outdated.

Other than that, the code and behavior LGTM

Edit: I would still like to see the Stepped option added here, but I will open an issue for it.

shahzad31 · 2021-03-26T07:10:59Z

While testing this I found a bug: using the Area Percentage chart type with a histogram shows this set of messages:

That was a good catch, pushed a fix for that.

kibanamachine · 2021-03-26T09:25:04Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	489	492	+3

Async chunks

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

id	before	after	diff
`lens`	928.8KB	931.3KB	+2.5KB

History

💚 Build #115227 succeeded d04e480
💚 Build #115106 succeeded e85d925
💔 Build #114980 failed 6b6ebf7
💔 Build #114966 failed 1e1b3c7
💚 Build #114792 succeeded bbec9f7

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

cc @shahzad31

wylieconlon

LGTM

Discussed offline, no objections.

…c#94675)

kibanamachine · 2021-03-26T15:27:18Z

💚 Backport successful

✅ 7.x / #95536

This backport PR will be merged automatically after passing CI.

#95536) Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>

shahzad31 added 3 commits March 16, 2021 08:48

wip

28ae14a

Merge branch 'master' into lens-chart-curve

28d27e4

pass state to xy visuilization

d23f615

shahzad31 marked this pull request as ready for review March 16, 2021 10:02

shahzad31 requested a review from a team March 16, 2021 10:02

shahzad31 self-assigned this Mar 16, 2021

shahzad31 added the Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability label Mar 16, 2021

shahzad31 added release_note:enhancement v7.13.0 v8.0.0 labels Mar 16, 2021

shahzad31 requested a review from flash1293 March 16, 2021 10:04

shahzad31 added 2 commits March 21, 2021 22:05

Merge branch 'master' into lens-chart-curve

62c42c7

update to curverd/non-curved

faf534d

flash1293 added Feature:Lens Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Mar 22, 2021

MichaelMarcialis reviewed Mar 22, 2021

View reviewed changes

Merge branch 'master' into lens-chart-curve

6d42a5b

markov00 reviewed Mar 23, 2021

View reviewed changes

shahzad31 added 3 commits March 23, 2021 12:06

update tests

5443816

fix i18n

796ef43

snapshot

9b8e8bc

flash1293 reviewed Mar 24, 2021

View reviewed changes

shahzad31 added 6 commits March 24, 2021 16:21

Merge branch 'master' into lens-chart-curve

fdd51a3

update dir location and store string values

1e1b3c7

fix type

6b6ebf7

Merge branch 'master' into lens-chart-curve

979be49

update snaps

d9bc6f4

update snapshots

e85d925

shahzad31 requested a review from flash1293 March 25, 2021 09:50

flash1293 previously requested changes Mar 25, 2021

View reviewed changes

shahzad31 added 2 commits March 25, 2021 16:36

Merge branch 'master' into lens-chart-curve

ca1833e

keep curve type state

d04e480

shahzad31 requested a review from flash1293 March 25, 2021 16:02

wylieconlon suggested changes Mar 25, 2021

View reviewed changes

shahzad31 added 2 commits March 26, 2021 08:03

Merge branch 'master' into lens-chart-curve

9a62539

fix tooltip help condition

c96edb7

shahzad31 requested a review from wylieconlon March 26, 2021 10:19

wylieconlon approved these changes Mar 26, 2021

View reviewed changes

shahzad31 merged commit b1525f0 into elastic:master Mar 26, 2021

shahzad31 deleted the lens-chart-curve branch March 26, 2021 15:23

shahzad31 added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 26, 2021

kibanamachine mentioned this pull request Mar 26, 2021

[7.x] [Lens] Allow modifying curve type for line/area series charts (#94675) #95536

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 26, 2021

[Lens] Allow modifying curve type for line/area series charts (elasti…

2c988e9

…c#94675)

kibanamachine added a commit that referenced this pull request Mar 26, 2021

[Lens] Allow modifying curve type for line/area series charts (#94675) (

0abdd0c

#95536) Co-authored-by: Shahzad <shahzad.muhammad@elastic.co>

Conversation

shahzad31 commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Mar 16, 2021

Uh oh!

flash1293 commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markov00 commented Mar 16, 2021

Uh oh!

flash1293 commented Mar 16, 2021

Uh oh!

shahzad31 commented Mar 17, 2021

Uh oh!

shahzad31 commented Mar 22, 2021

Uh oh!

elasticmachine commented Mar 22, 2021

Uh oh!

flash1293 commented Mar 22, 2021

Uh oh!

MichaelMarcialis left a comment

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!

markov00 Mar 23, 2021 • 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!

shahzad31 commented Mar 23, 2021

Uh oh!

shahzad31 commented Mar 24, 2021

Uh oh!

shahzad31 commented Mar 24, 2021

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!

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!

flash1293 commented Mar 25, 2021

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

wylieconlon 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!

shahzad31 commented Mar 26, 2021

Uh oh!

kibanamachine commented Mar 26, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

wylieconlon left a comment

shahzad31 commented Mar 16, 2021 •

edited

Loading

flash1293 commented Mar 16, 2021 •

edited

Loading

markov00 Mar 23, 2021 •

edited

Loading

flash1293 left a comment •

edited

Loading

wylieconlon left a comment •

edited

Loading