[Lens] Color in dimension trigger #76871

flash1293 · 2020-09-07T12:26:40Z

Based on #79851

This unfinished PR adds color indicators for dimension in xy chart.

Please don't review in depth yet, this PR is not cleaned up yet.

Feature

The indicators are shown when possible. If the color can't be determined (because "break down by" is used), the "disabled" icon is shown. For dimensions containing a color palette picker, the currently picked palette is shown in the dimension picker.

The color indicator does not contain a color picker itself - to change the color you still have to open the dimension popover. As soon as the flyout dimension config PR lands, I think this is justifiable as it's just a single click away.

Implementation

This PR extends the chart config like this: https://github.com/elastic/kibana/pull/76871/files#diff-c46757b63dd572e7cff7c0aad72c3ec135b9969c39716d4db9df551fa962ce34R347-R361

When passing the accessors to the frame, it's possible to specify a color icon as well. Another option would be to provide a renderColorIndicator or something along those lines (maybe making it more than generic than being about colors) and using a separate NativeRenderer. This would give the visualization more flexibility - I thought it would add unnecessary complexity in this specific situation, but I don't hold this opinion strongly.

The logic to determine the current color of a dimension (https://github.com/elastic/kibana/pull/76871/files#diff-5dcc03dab8d34631c6ed65b6b38f8b97eed0394699532c6516111c735a8e30bfR177-R220) works like this:

Check whether currently rendered data is available - if not, don't show an indicator
Check whether current layer has a break down by dimension - if yes, show the disabled icon
Check whether there is an override color - if yes, show that
Use the same color assignment logic as the actual chart to determine the current series color

Quick note about layout: To make the color indicator clickable like the dimension trigger rendered by the datasource, the EuiLink component rendering the button is moved out of the datasource into the frame - this means the datasource only has to render the text instead of having to handle the logic to actually open the popover: https://github.com/elastic/kibana/pull/76871/files#diff-9d5c4c19a499161633fc439fb1b86a6105bf7b2af2cdf638c52cec4f2a3c4f3cL93

Other changes:

Make the color assignment logic resilient to malformed data and make it work with both expression level types and state level types: x-pack/plugins/lens/public/xy_visualization/color_assignment.ts
Fix a potential bug in color assignment logic where the default palette was not picked correctly https://github.com/elastic/kibana/pull/76871/files#diff-1ca248a5e9aa96a613a49b87904efc6ce9e931e496f3f4dd7ccd15aa580d8e61R29
Extract column label to accessor map generation out of the expression function to be able to use it for coloring logic as well: https://github.com/elastic/kibana/pull/76871/files#diff-95f4d1c35bb73bd6c00ce53520dcdb9a3ac2db7707fae067542fd218e9f9a36dR42

…ension-trigger

…e-data

…rigger

…e-data

…rigger

…ension-trigger

flash1293 · 2020-11-09T07:37:04Z

@elasticmachine merge upstream

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx

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

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

mbondyra · 2020-11-10T13:49:53Z

What an amazing PR 😻

From small things I noticed:

This PR changed slightly the styles for focused dimension trigger:

Opinion land:

I wonder if in the case below we want to display the the little square logo for yAxis at all... I feel like skipping it would be less confusing. Or maybe we could add a tooltip to explain what it means?

flash1293 · 2020-11-10T14:14:17Z

Pinging @MichaelMarcialis for the style of color palettes inside of dimension triggers and Martas suggestion here: #76871 (comment)

I see the point, I was following the mocks here but I think just not showing anything for splitted y axis metrics is better.

MichaelMarcialis · 2020-11-10T22:21:08Z

This is looking great, @flash1293. While comparing your screenshots to @cchaos's latest (I think) mockups for chart coloring, I noticed that there were a few places that this PR deviates from those designs, including:

The brush icon appears to be used, where the mockups use color.
The stopSlash icon appears to be used, where the latest mockups appear to use no icon when axis color indicators are removed due to a breakdown palette being added.
The brush icon uses $euiLinkColor, where the mockups use the default text color.
The color bar placement here is flush with the left/bottom/right of its container, but is padded within its container in the mockups.

Was there a reason for these deviations? Am I looking at an older mockup, or is this just the first part of a multi-PR effort (as your initial comment appears to indicate)? Thanks!

MichaelMarcialis · 2020-11-10T22:48:46Z

This PR changed slightly the styles for focused dimension trigger:

Is there any way we can fix this so that the focus states updated as part of @mbondyra's recent reordering PR can remain intact? Figma examples of what was she implemented can be found here: https://www.figma.com/file/q61JmGHmcammPfkxupNj1g/Intradimensional-Reordering-Version-1-In-Progress?node-id=1%3A2

I wonder if in the case below we want to display the the little square logo for yAxis at all... I feel like skipping it would be less confusing. Or maybe we could add a tooltip to explain what it means?

Per my earlier comment, @cchaos's most recent mockups appear to omit this stopSlash icon altogether (unless I'm looking at an outdated file). Personally, after having seen it both ways, I tend to like the inclusion of the icon because it:

Provides consistency between dimensional items, in terms of having a preceding icon, having the text aligned the same across items, etc.
Is potentially helpful for users to quickly identify existing or possible areas of color control.

Regarding applying a tooltip to these stopSlash icons, I'm a bit hesitant to recommend that direction, as there's already a lot interaction already attached to each dimensional item. Most importantly, I don't wish to potentially interfere with the drag-and-drop (i.e. hovering the icon opens a tooltip, which inadvertently prevents a portion of the item from being draggable). I wonder if simply using a lower contrast icon color (such as $euiColorMediumShade) would put less of a spotlight on it and better indicate that it's a swatch chip in a disabled/empty state. Perhaps we could also explain further in the item's color field in the configuration flyout that it's being overridden. Thoughts?

…ension-trigger

flash1293 · 2020-11-11T17:42:12Z

Thanks for the reviews. I tried to address all of the things, this is the summary:

Code:
- Moved color logic into separate components/functions
- Removed the ability to specify an accessor config as string (you are right Marta, not really necessary)
Behavior:
- Using text color for brush
- Fixing focus style
- Using subdued color for stopSlash

@MichaelMarcialis

Personally, after having seen it both ways, I tend to like the inclusion of the icon

I left it in with a little less prominent color. No strong opinion here.

About the differences not addressed above:

The brush icon appears to be used, where the mockups use color.

I didn't find the right icon, maybe it's not released yet? If I'm using <EuiIcon type="color", I don't get an icon at all. Am I looking in the wrong place?

The color bar placement here is flush with the left/bottom/right of its container, but is padded within its container in the mockups.

I did that as a quick and dirty solution, but then I ended up liking it because it's not adding to the vertical space, preventing scrolling for a little bit longer. If you think it's not the right thing I can move to the style in Carolines mock.

MichaelMarcialis

Hey, @flash1293. Took a deeper look at the code and left a few comments for your review.

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/color_indicator.tsx

MichaelMarcialis · 2020-11-12T23:01:00Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/palette_indicator.tsx

+    <EuiFlexGroup className="lnsLayerPanel__paletteContainer" gutterSize="none" alignItems="center">
+      {accessorConfig.palette.map((color) => (
+        <EuiFlexItem
+          key={color}
+          className="lnsLayerPanel__paletteColor"
+          grow={true}
+          style={{
+            backgroundColor: color,
+          }}
+        />
+      ))}
+    </EuiFlexGroup>


Rather than rolling out a custom color palette component, would it be better to use EuiColorPaletteDisplay instead (and override as necessary if needed)?

https://elastic.github.io/eui/#/forms/color-selection

This component is very new - the version supporting it is not available in Kibana yet. I'm fine with using it once it becomes available, but #82730 has to get merged first.

Sounds good. Should I open an issue as a reminder? Or is that unnecessary?

Don't bother, I will take care of it.

MichaelMarcialis · 2020-11-12T23:32:08Z

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

@@ -86,11 +86,11 @@ export const IndexPatternDimensionTriggerComponent = function IndexPatternDimens
        }
        anchorClassName="eui-displayBlock"
      >
-        <EuiLink
+        <EuiText


There are a few questions/concerns I have due to the change from EuiLink to EuiText:

Since this element is no longer focusable, the styles applied by @mbondyra in a previous PR no longer set the focus styles (light blue background color behind text) appropriately for the child .lnsLayerPanel__triggerLinkLabel element. Can this be corrected?

I believe this aria-label now no longer applies to this element. Can we move to the appropriate location?

Should the lnsLayerPanel__triggerLink class name be changed, since it's no longer a link?

…ension-trigger

flash1293 · 2020-11-16T09:59:02Z

Thanks for the review, @MichaelMarcialis

These are the changes I made:

Use eui icon for solid color indicators as well
Set aria labels for them
Didn't change palette to the EUI implementation as this component is not available in Kibana yet
Fixed focus styles (I hope :) ) - I moved the focus style logic to the right class
Renamed lnsLayerPanel__triggerLink to lnsLayerPanel__triggerText as the link part is now handled by the frame in lnsLayerPanel__dimensionLink- move aria label / title to the button element again as well

kibanamachine · 2020-11-16T11:30:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4e0a21f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	433	435	+2

Async chunks

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

id	before	after	diff
`lens`	917.9KB	924.7KB	+6.8KB

Page load bundle

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

id	before	after	diff
`lens`	50.7KB	52.4KB	+1.7KB

History

💚 Build #87397 succeeded 188ed08
💚 Build #86644 succeeded d32cb66
💚 Build #86508 succeeded 61e4849

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

MichaelMarcialis

Thank you for addressing those suggested changes. This LGTM.

MichaelMarcialis · 2020-11-16T16:27:50Z

The color bar placement here is flush with the left/bottom/right of its container, but is padded within its container in the mockups.

I did that as a quick and dirty solution, but then I ended up liking it because it's not adding to the vertical space, preventing scrolling for a little bit longer. If you think it's not the right thing I can move to the style in Carolines mock.

Personally, I like how you've positioned the palette here and can appreciate the vertical real estate savings. Was just checking to see if there was a different reason for the deviation. I say we keep it positioned as you currently have it.

mbondyra

tested on Safari, works as expected. Code LGTM 🟢

flash1293 · 2020-11-17T09:19:07Z

Merging now, I will replace the custom palette code with the EUI component once it becomes available

* master: (51 commits) [ML] Persisted URL state for the Data frame analytics jobs and models pages (elastic#83439) adds xpack.security.authc.selector.enabled setting (elastic#83551) skip flaky suite (elastic#77279) [ML] Improve support for script and aggregation fields in anomaly detection jobs (elastic#81923) [Workplace Search] Migrate SourcesLogic from ent-search (elastic#83544) [ML] Add UI test for feature importance features (elastic#82677) [Maps] Improve icons for all layer types (elastic#83503) Replace experimental badge with Beta (elastic#83468) [Fleet][EPM] Unified install and archive (elastic#83384) Move src/legacy/server/keystore to src/cli (elastic#83483) Used SO for saving the API key IDs that should be deleted (elastic#82211) [Uptime] Mock implementation to account for math flakiness test (elastic#83535) [Workplace Search] Enable check for org context based on URL (elastic#83487) [App Search] Added all Document related routes and logic (elastic#83324) [Alerting UI] Fix console error when setting connector params (elastic#83333) [Discover] Allow custom name for fields via index pattern field management (elastic#70039) [Uptime] Fix monitor list down histogram (elastic#83411) remove headers timeout hack, rely on nodejs timeouts (elastic#83419) [ML] Update console autocomplete for ML data frame evaluate API (elastic#83151) [Lens] Color in dimension trigger (elastic#76871) ...

flash1293 added 2 commits September 7, 2020 14:19

implement color indicators

dc03598

add icon for color by

1118d0c

flash1293 added Feature:Lens v7.10.0 v8.0.0 release_note:enhancement labels Sep 7, 2020

flash1293 added 2 commits September 7, 2020 15:01

fix styling

66f4bd1

Merge remote-tracking branch 'upstream/master' into lens/color-in-dim…

5d0192e

…ension-trigger

wylieconlon added v7.11.0 and removed v7.10.0 labels Sep 24, 2020

wylieconlon mentioned this pull request Oct 1, 2020

[Lens] Lens editor should be able to use the active data in configuration #79194

Closed

flash1293 added 17 commits October 7, 2020 16:26

expose active data in some places

d5ba1b3

Merge remote-tracking branch 'upstream/master' into lens/expose-activ…

be3dd02

…e-data

add active data to all relevant places

696f791

Fix bugs

1490d31

fix types

f3ca265

add tests

a78946a

Merge remote-tracking branch 'upstream/master' into lens/expose-activ…

c751be7

…e-data

remove leftover

8a695d1

add test for expression renderer component

3820cfa

Merge branch 'lens/expose-active-data' into lens/color-in-dimension-t…

4de8419

…rigger

add series and palette indicator

9e4fb2e

Merge remote-tracking branch 'upstream/master' into lens/expose-activ…

2de1df2

…e-data

update docs

ec31106

Merge branch 'lens/expose-active-data' into lens/color-in-dimension-t…

4d708bc

…rigger

put stubs in place for things to test

b58abd4

Merge remote-tracking branch 'upstream/master' into lens/color-in-dim…

9cff0b2

…ension-trigger

fix tests and stuff

9c80ec2

elastic deleted a comment from cchaos Nov 5, 2020

elastic deleted a comment from AlonaNadler Nov 5, 2020

Merge branch 'master' into lens/color-in-dimension-trigger

d32cb66

mbondyra reviewed Nov 10, 2020

View reviewed changes

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

mbondyra reviewed Nov 10, 2020

View reviewed changes

x-pack/plugins/lens/public/types.ts Outdated Show resolved Hide resolved

mbondyra reviewed Nov 10, 2020

View reviewed changes

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

flash1293 requested a review from MichaelMarcialis November 10, 2020 14:13

flash1293 added 3 commits November 11, 2020 17:30

Merge remote-tracking branch 'upstream/master' into lens/color-in-dim…

723a3db

…ension-trigger

review comments

8ed06f8

simplify

188ed08

MichaelMarcialis reviewed Nov 12, 2020

View reviewed changes

flash1293 added 3 commits November 16, 2020 09:48

merge remote-tracking branch 'upstream/master' into lens/color-in-dim…

b9b6dc5

…ension-trigger

review fixes

82580bf

remove unused css class

4e0a21f

MichaelMarcialis approved these changes Nov 16, 2020

View reviewed changes

wylieconlon self-requested a review November 17, 2020 00:28

mbondyra approved these changes Nov 17, 2020

View reviewed changes

flash1293 merged commit 95e44f2 into elastic:master Nov 17, 2020

flash1293 added a commit to flash1293/kibana that referenced this pull request Nov 17, 2020

[Lens] Color in dimension trigger (elastic#76871)

890ffdb

flash1293 mentioned this pull request Nov 17, 2020

[7.x] [Lens] Color in dimension trigger (#76871) #83524

Merged

flash1293 added a commit that referenced this pull request Nov 17, 2020

[Lens] Color in dimension trigger (#76871) (#83524)

495fc8b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Color in dimension trigger #76871

[Lens] Color in dimension trigger #76871

flash1293 commented Sep 7, 2020 •

edited

Loading

flash1293 commented Nov 9, 2020

mbondyra commented Nov 10, 2020 •

edited

Loading

flash1293 commented Nov 10, 2020 •

edited

Loading

MichaelMarcialis commented Nov 10, 2020

MichaelMarcialis commented Nov 10, 2020

flash1293 commented Nov 11, 2020 •

edited

Loading

MichaelMarcialis left a comment

MichaelMarcialis Nov 12, 2020

flash1293 Nov 16, 2020

MichaelMarcialis Nov 16, 2020

flash1293 Nov 16, 2020

MichaelMarcialis Nov 12, 2020

flash1293 commented Nov 16, 2020

kibanamachine commented Nov 16, 2020

MichaelMarcialis left a comment

MichaelMarcialis commented Nov 16, 2020

mbondyra left a comment

flash1293 commented Nov 17, 2020

[Lens] Color in dimension trigger #76871

[Lens] Color in dimension trigger #76871

Conversation

flash1293 commented Sep 7, 2020 • edited Loading

Feature

Implementation

flash1293 commented Nov 9, 2020

mbondyra commented Nov 10, 2020 • edited Loading

Opinion land:

flash1293 commented Nov 10, 2020 • edited Loading

MichaelMarcialis commented Nov 10, 2020

MichaelMarcialis commented Nov 10, 2020

flash1293 commented Nov 11, 2020 • edited Loading

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Nov 12, 2020

Choose a reason for hiding this comment

flash1293 Nov 16, 2020

Choose a reason for hiding this comment

MichaelMarcialis Nov 16, 2020

Choose a reason for hiding this comment

flash1293 Nov 16, 2020

Choose a reason for hiding this comment

MichaelMarcialis Nov 12, 2020

Choose a reason for hiding this comment

flash1293 commented Nov 16, 2020

kibanamachine commented Nov 16, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis commented Nov 16, 2020

mbondyra left a comment

Choose a reason for hiding this comment

flash1293 commented Nov 17, 2020

flash1293 commented Sep 7, 2020 •

edited

Loading

mbondyra commented Nov 10, 2020 •

edited

Loading

flash1293 commented Nov 10, 2020 •

edited

Loading

flash1293 commented Nov 11, 2020 •

edited

Loading