[Lens] Improve color stop UI by VladLasitsa · Pull Request #119165 · elastic/kibana

VladLasitsa · 2021-11-19T14:16:09Z

Closes: #116422

Summary

This PR changes UI of color stops in Lens.

How it looks now:

Also was added new functionality:

distribute values - allow user to distribute ranges equally between max and min.
auto detect man and max values - allow user to auto detect man and max values for currect state of data.

Tasks:

add new and fix existing unit tests
fix existing functional tests
fix behavior with max/min values in heatmap
doesn't keep changed first range between percentage and number mods

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

dej611

Had a first round of code review.
Locally I've test it and managed to break it only on the metric visualization. 👍

Test the metric visualization, which makes an intense use of the rangeMin/rangeMax props.

x-pack/plugins/lens/public/shared_components/color_palette/palette_configuration.tsx

x-pack/plugins/lens/public/shared_components/color_palette/color_ranges.tsx

dej611 · 2021-11-30T09:57:07Z

x-pack/plugins/lens/public/shared_components/coloring/utils.ts

    return {
      color,
-      stop: newMin + ((stop - oldMin) * newInterval) / oldInterval,
+      stop: newMin + ((stop - oldMin) * 100 * newInterval) / 100 / oldInterval,


I think the * 100 / 100 can be removed here as there's no actual change.

It help to fix some problems with correct rounding

x-pack/plugins/lens/public/shared_components/coloring/palette_configuration.tsx

dej611 · 2021-11-30T10:01:21Z

x-pack/plugins/lens/public/shared_components/color_palette/color_ranges.tsx

+              start: roundValue(start + (step * 100 * index) / 100),
+              end: roundValue(start + (step * 100 * (index + 1)) / 100),


Also here I think the * 100 / 100 are superfluous. The roundValue will apply already some internally

no because firstly we should use *100 and after that * index / 100 in another case we have problem. You can check it try with that expression 11596411699.2 * 3 * 100 / 100 and 11596411699.2 * 100 * 3/ 100 you will get different results. And the second more correct.

But that's not a different result, it is just how float values work. But even when slightly different after the 5th digit, the roundValue will take care of it.

sometimes I got .5999999 instead of .60000 if i use the first options, with the second always .6000.

x-pack/plugins/lens/public/shared_components/color_palette/color_ranges.tsx

markov00 · 2021-12-06T09:37:28Z

@VladLasitsa just a minor comment on the SVG icons: you can reduce the size by half, most of the time, if you use https://jakearchibald.github.io/svgomg/ to cleanup the SVG path: the resulting SVG is exactly the same but optimized

VladLasitsa · 2021-12-07T12:40:06Z

@VladLasitsa just a minor comment on the SVG icons: you can reduce the size by half, most of the time, if you use https://jakearchibald.github.io/svgomg/ to cleanup the SVG path: the resulting SVG is exactly the same but optimized

Thank you @markov00, I am used this tool and decrease size of all my SVGs.

flash1293 · 2022-01-21T11:02:24Z

@elasticmachine merge upstream

flash1293 · 2022-01-21T11:20:42Z

Doesn't work for numbers with a decimal point:

Edit: See the stuff below, it's probably related to the same issue about max not being handled correctly

flash1293 · 2022-01-21T11:22:44Z

The thing above is also an issue for percentage-based palettes - I think it always happens if the value the percentage evaluates to has digits after the decimal point, but it's also possible to completely confuse it:

flash1293 · 2022-01-21T11:25:19Z

Seems like max value doesn't work:

flash1293 · 2022-01-21T11:26:10Z

Same for explicit max value instead of data bounds:

flash1293 · 2022-01-21T11:34:32Z

For metric specifically, absolute min value works but data-bounds based min value makes it always use the min value:

It should just behave like minus infinity

flash1293 · 2022-01-21T11:38:15Z

When distributing equally, it rounds all of the intermediary things, but not the min (use an average metric to get values with long tails):

flash1293 · 2022-01-21T11:43:56Z

On selecting the "custom" palette again from the dropdown if it's custom already it does very weird things - it removes the first stop and adds an additional stop in the end (0 - 80 before, to 20 - 100 afterwards):

flash1293 · 2022-01-21T15:47:00Z

@VladLasitsa first two tests look unrelated flakiness, the last one is probably legit

Jest Tests #5 / metric_expression MetricChart component it renders the correct color styling for numeric value if user select auto detect max value

VladLasitsa · 2022-01-24T07:55:11Z

@elasticmachine merge upstream

kibana-ci · 2022-01-24T09:15:50Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Default CI Group #18 / apis Maps endpoints maps_telemetry should return the correct telemetry values for map saved objects

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`charts`	76	77	+1
`lens`	718	735	+17
total			+18

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`charts`	281	286	+5

Async chunks

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

id	before	after	diff
`charts`	41.8KB	42.1KB	+347.0B
`expressionHeatmap`	15.0KB	15.0KB	+16.0B
`lens`	1.0MB	1.0MB	+11.6KB
total			+11.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`charts`	58.0KB	58.3KB	+262.0B

Unknown metric groups

API count

id	before	after	diff
`charts`	314	319	+5

ESLint disabled line counts

id	before	after	diff
`lens`	39	38	-1

Total ESLint disabled count

id	before	after	diff
`lens`	41	40	-1

History

💚 Build #18929 succeeded 84bbe33
💔 Build #18896 failed 262a2ea
💔 Build #18895 failed fc02977
💔 Build #18889 failed 9e0ddd9
💔 Build #18880 failed 67194be
💚 Build #18472 succeeded 2a11580

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

cc @VladLasitsa @alexwizp

flash1293

A few nits I found but let's split them out into a follow-up issue:

On metric, turning off auto max value will not round the value:

On metric, turning off auto min value will log a warning: `The specified value "-Infinity" cannot be parsed, or is out of range.

Besides that it looks good to me - @dej611 @stratoula could you give it another look as well?

stratoula

This works great. I only found the following behavior that seems weird to me and if I am not mistaken it happens only on heatmaps.

When I am on the percentage mode and change the max value from 100 to 87 for example, this change is not depicted in the chart

If I change to Number, this works fine

flash1293 · 2022-01-25T11:01:42Z

Good catch @stratoula - I think we can merge in this PR and fix this problem along with my nits in a follow-up (to make sure it's safe and sound before FF)

stratoula

Definitely @flash1293. I am approving as we are going to have a followup PR!

dej611

Code review only

MichaelMarcialis · 2022-01-25T18:37:31Z

Hey gang. I'm only just now seeing this PR (got a notification because it closed out the originating issue). Really great work here! I'm excited to see these enhancements added.

I know this has already been merged, but in testing after the fact, I had a handful of comments/questions. Would we be able to address the following in a follow-up PR?

Adding new color to the color ranges currently increments the new color's value by +25 from the last color value. Can we change this to increment by +1, per the designs (to make it less likely to create a value that is invalid in percentage mode)?
Can we change the wording of the Add color range button to Add color and Distribute equally button to Distribute values to better match the design (and reduce the likelihood of the buttons breaking into two lines)?
The percent symbol append isn't necessary for the EuiFieldText components that are set to be the current data's min or max value. Can we conditionally remove those append percent symbols in those cases?
It looks like the tooltips and accompanying text from the designs didn't get applied to the min, max, and pencil icon buttons. Can we add these in to match the designs, as shown here: https://www.figma.com/file/kWrDcUFIPsD6qwYO5bouju/Exploration-%2F-Version-4-%5BAwaiting-Dev%5D?node-id=114%3A121407
The color picker popover should support changing the opacity, as shown in the designs. Was the omission here intentional? I suppose there is an argument to be made for not supporting color opacity when the text is being colored (in table and metric type visualizations), but for all other instances, I don't believe there is a reason to exclude it.
Disabled color range action buttons are currently missing their suggested tooltips. Would it be possible to add these in to match the designs, as shown here: https://www.figma.com/file/kWrDcUFIPsD6qwYO5bouju/Exploration-%2F-Version-4-%5BAwaiting-Dev%5D?node-id=114%3A121018
Why are we warning users when they select a value that is lower than the current minimum data value or higher than the current maximum data value? Isn't that the point of applying a custom value (not being beholden to the current set of data values)? I feel like we should remove this warning, as it will likely give users the impression they're doing something wrong when they're not at all. Thoughts?

When a value is made to be greater than the currently applied maximum color range value, this PR shows an error to the user. Per our design discussions, this shouldn't be an error, but should instead follow the flow shown here: https://www.figma.com/file/kWrDcUFIPsD6qwYO5bouju/Exploration-%2F-Version-4-%5BAwaiting-Dev%5D?node-id=12%3A109769

Can we change the Maximum value should be greater than preceding values error message to be Maximum value must be greater than preceding values. to better match designs.
This PR currently shows gauge color band configuration as being unable to apply a custom value to the min/max values in the range. Is this intentional? Why wouldn't we allow the users to customize these values (as shown in the designs)?

This PR includes the option to toggle between min/max and custom values for metric type visualizations. How does this work (as metric type visualizations are a single value and not a range of values)? My assumption was that metric type visualizations would only offer custom values, with the default values being applied as described here: https://www.figma.com/file/kWrDcUFIPsD6qwYO5bouju/Exploration-%2F-Version-4-%5BAwaiting-Dev%5D?node-id=10%3A110344

CCing @ghudgins and @flash1293, in case they have any comments on the above.

flash1293 · 2022-01-25T18:45:40Z

Thanks for these @MichaelMarcialis , I will pull these into a separate issue together with my comments above, we can discuss there.

First very draft version

a56b23e

VladLasitsa requested a review from markov00 November 19, 2021 14:16

Added validation, clean up code

9e7715b

VladLasitsa requested a review from alexwizp November 24, 2021 10:50

Some fixes

7029192

dej611 reviewed Nov 24, 2021

View reviewed changes

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx Outdated Show resolved Hide resolved

Adapt components to the new UI design

e10e350

VladLasitsa requested a review from dej611 November 26, 2021 14:33

VladLasitsa added 2 commits November 29, 2021 16:47

Some fixes

924efe3

Fix validation

bc9d87f

VladLasitsa requested a review from DianaDerevyankina November 29, 2021 14:13

dej611 reviewed Nov 30, 2021

View reviewed changes

VladLasitsa added 4 commits November 30, 2021 14:55

Fix lint errors

be37180

Fix metric vis for new color stop UI

c3d0786

Fix problems with keeping state of auto detecting max/min value

fea83b3

Add tests

35ad9b3

VladLasitsa added 8 commits December 6, 2021 13:40

Fix CI

2f9b2f1

Fix tests

a9017f8

Fix some lint problems

212dd76

Fix CI

ebc6a3a

Merge remote-tracking branch 'upstream/main' into 116422

4ea7daa

Fix min/max behavior for heatmap

5765073

Fix checks.

c863aa4

Fix auto value when we add new color range

956b3ba

VladLasitsa requested a review from dej611 December 7, 2021 12:58

VladLasitsa added 3 commits December 7, 2021 16:05

Fix check task

084461e

Fix some issues

7dd40d0

Some fixes

93927da

VladLasitsa added 3 commits January 21, 2022 10:19

Fix comments

daa5aaa

Some changes

67194be

Fix tests

6aa3aa8

Merge branch 'main' into 116422

9e0ddd9

VladLasitsa added 2 commits January 21, 2022 15:49

Fix all comments

fc02977

Fix Checks

262a2ea

Fix CI

84bbe33

Merge branch 'main' into 116422

1b323d3

alexwizp requested a review from flash1293 January 24, 2022 09:29

flash1293 reviewed Jan 24, 2022

View reviewed changes

flash1293 approved these changes Jan 24, 2022

View reviewed changes

stratoula reviewed Jan 25, 2022

View reviewed changes

stratoula approved these changes Jan 25, 2022

View reviewed changes

dej611 approved these changes Jan 25, 2022

View reviewed changes

alexwizp merged commit 95f2967 into elastic:main Jan 25, 2022

nickofthyme mentioned this pull request Jul 12, 2024

Color ranges break with Last value for array of values #188263

Open

		start: roundValue(start + (step * 100 * index) / 100),
		end: roundValue(start + (step * 100 * (index + 1)) / 100),

Conversation

VladLasitsa commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Uh oh!

dej611 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dej611 Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

VladLasitsa Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dej611 Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

VladLasitsa Nov 30, 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!

dej611 Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

VladLasitsa Nov 30, 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!

Uh oh!

markov00 commented Dec 6, 2021

Uh oh!

VladLasitsa commented Dec 7, 2021

Uh oh!

flash1293 commented Jan 21, 2022

Uh oh!

flash1293 commented Jan 21, 2022 • edited by VladLasitsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Jan 21, 2022 • edited by VladLasitsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Jan 21, 2022 • edited by VladLasitsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Jan 21, 2022 • edited by VladLasitsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Jan 21, 2022 • edited by VladLasitsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Jan 21, 2022 • edited by VladLasitsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Jan 21, 2022 • edited by VladLasitsa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flash1293 commented Jan 21, 2022

Uh oh!

VladLasitsa commented Jan 24, 2022

Uh oh!

kibana-ci commented Jan 24, 2022

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

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

Uh oh!

VladLasitsa commented Nov 19, 2021 •

edited

Loading

VladLasitsa Nov 30, 2021 •

edited

Loading

VladLasitsa Nov 30, 2021 •

edited

Loading

flash1293 commented Jan 21, 2022 •

edited by VladLasitsa

Loading

flash1293 commented Jan 21, 2022 •

edited by VladLasitsa

Loading

flash1293 commented Jan 21, 2022 •

edited by VladLasitsa

Loading

flash1293 commented Jan 21, 2022 •

edited by VladLasitsa

Loading

flash1293 commented Jan 21, 2022 •

edited by VladLasitsa

Loading

flash1293 commented Jan 21, 2022 •

edited by VladLasitsa

Loading

flash1293 commented Jan 21, 2022 •

edited by VladLasitsa

Loading

flash1293 left a comment •

edited

Loading