[XY axis] Integrates legend color picker with the eui palette #90589

stratoula · 2021-02-08T10:58:34Z

Summary

With the new XY axis implementation we give the users the ability to change the palette for these visualizations. The users can choose between the eui palettes and the legacy one.

The users can also overwrite the rendered colors by clicking on the legend color and apply a color. So far, they could choose between custom colors of the legacy palette.

With this PR the legend colors depend on the palette chosen. When the user chooses an EUI palette the legend colors are a superset of the default eui palette while if the user chooses the legacy palette the colors of the legend are the legacy ones.

I have used a rotation of 4.

User has chosen the legacy palette:

User has chosen the default eui palette:

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
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 · 2021-02-08T14:07:54Z

@miukimiu your feedback here is valuable! What do you think about the 4 rotations?

elizabetdev · 2021-02-09T11:44:16Z

Hi @stratoula ,

The 4 rotations are ok if the goal is to give to the user a similar number of colors as the legacy one. I think it would be weird to pick the legacy with so many colors and then the EUI palette with just a few. With this in mind, the 4 rotations sound good.

But in both scenarios, legacy, and default EUI a lot of colors are very similar, almost impossible to distinguish. So do we need so many colors? Maybe the Elastic Charts team can recommend us what is an appropriate number of colors and based on that, we can lower or increase the number of colors for all the palettes.

CC @nickofthyme @markov00

stratoula · 2021-02-09T12:06:44Z

This is the reason that I went with 4 rotations but it seems weird to me as I def agree that the colours are pretty similar.

nickofthyme · 2021-02-09T19:15:22Z

From my perspective, I think fewer rotations the better, so 4 is fine with me. We could also just do a single rotation and create larger color selections inside the popover, similar to the quick colors on the bottom of the colors picker. This would be the best approach IMHO.

if we do go with four rotations, I would like to see the popover width increase to align the rotations.

This creates a pretty wide popover so maybe we could transpose the matrix to be more vertical and have the rotated values go from left to right instead of top to bottom.

stratoula · 2021-02-11T06:44:38Z

@elasticmachine merge upstream

stratoula · 2021-02-11T08:15:54Z

I like your proposal @nickofthyme but what troubles me is that when the users choose the legacy palette will see a different UI. @miukimiu wdyt?

nickofthyme · 2021-02-11T14:14:16Z

@stratoula yeah agreed, this is a tough call that I'm glad I don't have to make 😄

stratoula · 2021-02-15T06:40:50Z

@elasticmachine merge upstream

markov00 · 2021-02-16T11:03:40Z

hey @stratoula @miukimiu @nickofthyme I'm not a designer but I hope this example can provide a different viewpoint on the color picker:

enabling the user to choose the palette directly from the color picker can be a useful but not required, less context change and more data exploration
providing the user with more than 10 colors is counterproductive and leads to bad visualizations. It is difficult for the user to choose a different color from the already selected in the legend if we are playing with > 10 colors. I know on the example there are 12 but the palette is 10 + 2 neutral, we can reduce it if we want.
The following goes against the previous point but is the reason why providing also variances/palettes on colors is a good thing: as described here [Discuss] tradeoffs on the single-palette guidance for data visualization eui#3742 color discernibility varies with the size and type of shapes used on the data visualization and providing a good single palette for every use case is probably very difficult.
On the example, I've also added a set of neutral colors (light gray, black) that can be beneficial for the user that likes to highlight a specific series while turning down the importance of others (note that we don't have such ability today because none of the palettes offer a set of neutral colors)
if the user really needs a different set of colors, we can offer a set of variation/rotation when interacting with the variation slider (I don't spend much time on the UX here but my idea is to let the user choose a different color rotation but this needs a bit more interaction to achieve it)
we should always open the color picker with the current selected color and palette
we can use the same color picker for every palette, no need to go back to the old circles one but the user can use the legacy palette for older charts. I think we can exclude that palette from newly created charts.
please keep in mind again that this example is not well designed nor it is something to follow completely, I think the above points are more important than this design, I'd like to get feedback from @miukimiu and @MichaelMarcialis if possible as we are all working on various color topics.
I'm not arguing in this context about the provided palettes or the number of steps on the diverging or sequential palettes, but we should consider that in the future

cc @monfera

stratoula · 2021-02-18T09:20:02Z

@elasticmachine merge upstream

MichaelMarcialis · 2021-02-18T22:45:49Z

I'd like to get feedback from @miukimiu and @MichaelMarcialis if possible as we are all working on various color topics.

@markov00: Here are some of my initial thoughts based on your comments. I'm coming into this conversation a bit sideways, so please feel free to schedule something on the calendar if you think further discussion is warranted.

providing the user with more than 10 colors is counterproductive and leads to bad visualizations. It is difficult for the user to choose a different color from the already selected in the legend if we are playing with > 10 colors. I know on the example there are 12 but the palette is 10 + 2 neutral, we can reduce it if we want.

I generally agree with this suggestion (i.e. a reduction color options offered lowers chances of user decision paralysis and helps avoid potentially poor choices). However, I do wonder about those advanced users who would desire to have greater control over their colors. For example, Grafana defaults to offering a limited selection of colors, but also has the option for users to make custom colors if they wish. It's tucked away in a separate tab, but it's available for those advanced users. Perhaps that is beyond the scope of this PR, but thought I'd mention it.

On the example, I've also added a set of neutral colors (light gray, black) that can be beneficial for the user that likes to highlight a specific series while turning down the importance of others (note that we don't have such ability today because none of the palettes offer a set of neutral colors)

I like this idea. Do you feel that two neutral colors would be sufficient for the majority of these use cases? Have you given any thought as to what should happen to these neutral colors if the user selects the "Gray" color palette? I imagine they'd need to be conditionally removed or incorporated in some way.

if the user really needs a different set of colors, we can offer a set of variation/rotation when interacting with the variation slider (I don't spend much time on the UX here but my idea is to let the user choose a different color rotation but this needs a bit more interaction to achieve it)

This is an interesting solution to avoid inundating the user with too many color options. Perhaps as an alternative to the "Variant" slider in your mockup, we could change it to a lightness/darkness slider. The hue and saturation stays true to the chosen color palette, but the user can adjust the lightness/darkness of the available colors.

we should always open the color picker with the current selected color and palette

The relationship between the legend-based color picker and the color palette field in the sidebar is interesting. I like the idea that the default colors presented in the legend-based picker being pulled from the chosen color palette. However, I'm not sure about the actually changing the entire palette from within the legend-based color picker popover (assuming that is what your mockup is suggesting). I worry the user might be confused as to whether they were changing the palette for the entire visualization or just that particular value.

we can use the same color picker for every palette, no need to go back to the old circles one but the user can use the legacy palette for older charts. I think we can exclude that palette from newly created charts.

If you mean in terms of being consistent in our usage of the latest color palette and color picker components, I agree.

stratoula · 2021-02-22T06:40:07Z

@elasticmachine merge upstream

stratoula · 2021-02-22T11:48:05Z

After syncing with @markov00 and @miukimiu we will go for this PR with @nickofthyme 's suggestion and I created another issue #92151 to initialize the discussions about how we can do it better in the future

stratoula · 2021-02-25T07:48:31Z

@elasticmachine merge upstream

stratoula · 2021-02-25T08:47:34Z

@nickofthyme I ordered them by group and I think that now we can keep the 8 columns width. It looks like that

If I order them by 4 columns it is not so good so I prefer it that way, wdyt?

stratoula · 2021-03-01T06:36:53Z

@elasticmachine merge upstream

ThomThomson

Code review only - Presentation team changes LGTM!

myasonik

Hey, sorry for the long delay in my review!

I wasn't able to use the legend color picker widget with my keyboard at all so there are some significant issues here. I left comments on how I think they should be addressed and some other issues but let me know if you have any questions.

src/plugins/charts/public/static/components/color_picker.tsx

src/plugins/vis_type_xy/public/utils/get_color_picker.tsx

src/plugins/charts/public/static/components/color_picker.tsx

myasonik · 2021-03-08T21:45:03Z

src/plugins/charts/public/static/components/color_picker.scss

@@ -11,6 +11,11 @@ $visColorPickerWidth: $euiSizeL * 8; // 8 columns
    transform: scale(1.4);
  }

+  // removes the grey background that appears on the first dot when the popover is open
+  &:focus {
+    background-color: $euiColorEmptyShade;


This looks ok because we have that big circle around the color currently in use but, at the same time, this whole widget doesn't seem to be keyboard accessible (at least in Firefox) so I wasn't able to test how this looks like on buttons that aren't the currently selected color.

src/plugins/charts/public/static/components/color_picker.tsx

stratoula · 2021-03-09T06:24:59Z

@elasticmachine merge upstream

stratoula · 2021-03-09T08:39:06Z

@myasonik thanx for your feedback, this picker exists for so many versions that I am happy that we could improve its accessibility issues with my PR. I tried to follow your comments, can you check it again?

myasonik

This looks a lot closer (thank you!) but still has a few concerns.

src/plugins/charts/public/static/components/color_picker.tsx

stratoula · 2021-03-10T06:42:26Z

@elasticmachine merge upstream

nickofthyme

Looks good, just one comment on the keyboard tabbing when the color picker is opened. Looks like the ownFocus is not working as expected.

src/plugins/vis_type_xy/public/vis_component.tsx

nickofthyme · 2021-03-11T20:17:42Z

src/plugins/vis_type_xy/public/utils/get_color_picker.tsx

+      <EuiWrappingPopover
+        isOpen
+        ownFocus
+        display="block"
+        button={anchor}
+        anchorPosition={getAnchorPosition(legendPosition)}
+        closePopover={onClose}
+        panelPaddingSize="s"
+      >
+        <ColorPicker
+          color={paletteName === 'kibana_palette' ? color : color.toLowerCase()}
+          onChange={handleChange}
+          label={seriesName}
+          useLegacyColors={paletteName === 'kibana_palette'}
+          colorIsOverwritten={colorIsOverwritten}
+          onKeyDown={onKeyDown}
+        />
+      </EuiWrappingPopover>


Not sure how this was affected, but there seems to be no focus on the color dots when you using keyboard actions to open the color picker.

Below I focus a legend item and then tab to the color, hit enter and then tabbing when the color picker is open does nothing. I'm pretty sure this was working before.

Screen.Recording.2021-03-11.at.02.15.08.PM.mp4

@myasonik might have already mentioned this above. Not sure how to fix it.

The dots are now a radio group so arrow keys move you back and forward rather than tab.

If we think this is tricky for users at large, we can think about alternatives but I think this is better than where the component was at previously.

That said, there doesn't seem to be focus state initially (i.e., after you use arrow keys once, a focus state is visible but the first time you open a popover there doesn't seem to be anything).

Oh I see, that's fine. Thanks for the explanation. Yeah would be nice to have an initial state to know it focused on the popover.

What do you mean to focus on the first dot when it opens? I don't think we want this. We want to focus on the active color. Like here:

On your screenshot below @myasonik there is the legacy palette. The current series color doesn't exist on the popover so for this reason you don't see an active dot.

I think it is wrong to focus on the first dot, this means that every time I open the popover, the first color is selected and the chart will automatically change color. We def don't want this.

Currently there is a :focus state and I think an :active or selected state. I think we should add back the focus state and auto focus the first color on focus of the color picker. See the recording below.

Screen.Recording.2021-03-15.at.09.19.58.AM.mp4

This was lost due to our change on custom input radio btns. I could add it with css but I am not sure about it. If you see Wylie's comment above, previously we had auto focus on the first element, which was quite confusing. I will sync with @myasonik offline to decide what we want to do here. Maybe we should revert to the previous tab navigation which works like that (taking under consideration that we want to change this behavior in the future):

myasonik

The first time I open the popover, I don't see a focus state at all. I'd expect the first color to be already focused.

Otherwise, this is looking better with every commit!

stratoula · 2021-03-12T15:45:58Z

@elasticmachine merge upstream

stratoula · 2021-03-15T08:05:38Z

@elasticmachine merge upstream

stratoula · 2021-03-16T06:49:33Z

@elasticmachine merge upstream

kibanamachine · 2021-03-16T08:49:27Z

💚 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
`visTypeVislib`	624.7KB	624.6KB	-53.0B
`visTypeXy`	119.8KB	119.4KB	-438.0B
total			-491.0B

Page load bundle

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

id	before	after	diff
`charts`	159.7KB	161.3KB	+1.6KB

History

💚 Build #113004 succeeded bf9c9c5
💚 Build #112948 succeeded 776b434
💚 Build #112443 succeeded d6b3f4f
💔 Build #112435 failed ba3d403
💚 Build #112372 succeeded 59c6687

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

myasonik

I was looking in the wrong place earlier 🤦 Everything seems to be working as expected now!

To reiterate: we did change how the thing works a little (notably, moved from tab stops for every color, to arrow keys) but we were able to improve screen reader support a lot in that change so I think it was worth it even if there are still some rough edges.

With all of this being somewhat temporary until the next update anyways, I'm happy to ship this as is!

…c#90589) * XY Axis, integrate legend color picker with the eui palette * Fix functional test to work with the eui palette * Order eui colors by group * Add unit test for use color picker * Add useMemo to getColorPicker * Remove the grey background from the first focused circle * Fix bug caused by comparing lowercase with uppercase characters * Fix bug on complimentary palette * Fix CI * fix linter * Use uppercase for hex color * Use eui variable instead * Changes on charts.json * Make the color picker accessible * Fix ci and tests * Allow keyboard navigation * Close the popover on mouse click event * Fix ci Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

#94738) * XY Axis, integrate legend color picker with the eui palette * Fix functional test to work with the eui palette * Order eui colors by group * Add unit test for use color picker * Add useMemo to getColorPicker * Remove the grey background from the first focused circle * Fix bug caused by comparing lowercase with uppercase characters * Fix bug on complimentary palette * Fix CI * fix linter * Use uppercase for hex color * Use eui variable instead * Changes on charts.json * Make the color picker accessible * Fix ci and tests * Allow keyboard navigation * Close the popover on mouse click event * Fix ci Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…-action * 'master' of github.com:elastic/kibana: (44 commits) Migrate the optimizer mixin to core (#94272) Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid (#92442) [ML] Anomaly Detection: Migrate validation messages links to use docLinks. (#94568) [Lists][Exceptions] - Adding basic linting, i18n and storybook support (#94772) [Fleet] Add test/fix for invalid/missing ids in bulk agent reassign (#94632) [Security Solution] [Cases] Move create page components and dependencies to Cases (#94444) [ML] Data Frame Analytics accessibility tests: fix flaky outlier creation test (#94735) [Security Solutions] Remove commented out old linter rules (#94753) [App Search] Role mappings migration part 2 (#94461) [CI] Update Backport action inputs to match updated ones (#94721) [chore] Remove the infra team from CODEOWNERS (#94740) [Connectors UI] Make UI use new connector APIs (#94180) [ML] Use indices options in anomaly detection job wizards (#91830) Remove `string` as a valid registry var type value (#94174) [Alerts] Replaces legacy es client with the ElasticsearchClient for alerts and triggers_actions_ui plugins. (#93364) [Reporting-CSV Export] Re-write CSV Export using SearchSource (#88303) chore(NA): upgrade bazel rules nodejs to v3.2.2 (#94726) [APM] Settings: Update layout and update/add descriptions (#94398) skip flaky suite (#94666) [XY axis] Integrates legend color picker with the eui palette (#90589) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/constants.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_context.tsx # x-pack/plugins/index_lifecycle_management/public/shared_imports.ts

XY Axis, integrate legend color picker with the eui palette

f18f82a

stratoula changed the title ~~XY Axis, integrate legend color picker with the eui palette~~ [XY Axis] Integrates legend color picker with the eui palette Feb 8, 2021

Fix functional test to work with the eui palette

e02cee2

stratoula changed the title ~~[XY Axis] Integrates legend color picker with the eui palette~~ [XY axis] Integrates legend color picker with the eui palette Feb 8, 2021

stratoula mentioned this pull request Feb 8, 2021

[TSVB] Allows users to change the series color from the legend #88801

Closed

9 tasks

Merge branch 'master' into change-legend-palette-colors

b3ec517

Merge branch 'master' into change-legend-palette-colors

e5c2658

Merge branch 'master' into change-legend-palette-colors

1392aab

Merge branch 'master' into change-legend-palette-colors

5fa51f5

stratoula mentioned this pull request Feb 22, 2021

[Lens] Change a series color from the legend #92151

Closed

kibanamachine and others added 2 commits February 25, 2021 02:48

Merge branch 'master' into change-legend-palette-colors

b3ddd5c

Order eui colors by group

1e80776

Add unit test for use color picker

ee35b6b

Merge branch 'master' into change-legend-palette-colors

8438e25

kibanamachine and others added 2 commits March 8, 2021 03:04

Merge branch 'master' into change-legend-palette-colors

3bc070f

Changes on charts.json

78bbab6

ThomThomson approved these changes Mar 8, 2021

View reviewed changes

myasonik suggested changes Mar 8, 2021

View reviewed changes

kibanamachine and others added 2 commits March 9, 2021 01:25

Merge branch 'master' into change-legend-palette-colors

a300819

Make the color picker accessible

b3c464d

Fix ci and tests

4cc3543

myasonik suggested changes Mar 9, 2021

View reviewed changes

src/plugins/charts/public/static/components/color_picker.tsx Outdated Show resolved Hide resolved

src/plugins/charts/public/static/components/color_picker.tsx Outdated Show resolved Hide resolved

kibanamachine and others added 4 commits March 10, 2021 01:42

Merge branch 'master' into change-legend-palette-colors

59c6687

Allow keyboard navigation

ba3d403

Close the popover on mouse click event

a21dfd6

Fix ci

d6b3f4f

nickofthyme reviewed Mar 11, 2021

View reviewed changes

myasonik reviewed Mar 11, 2021

View reviewed changes

Merge branch 'master' into change-legend-palette-colors

776b434

Merge branch 'master' into change-legend-palette-colors

bf9c9c5

Merge branch 'master' into change-legend-palette-colors

e0e3488

myasonik approved these changes Mar 16, 2021

View reviewed changes

stratoula merged commit a1f15fb into elastic:master Mar 16, 2021

stratoula mentioned this pull request Mar 16, 2021

[7.x] [XY axis] Integrates legend color picker with the eui palette (#90589) #94738

Merged

[XY axis] Integrates legend color picker with the eui palette #90589

[XY axis] Integrates legend color picker with the eui palette #90589

Conversation

stratoula commented Feb 8, 2021 • edited Loading

Summary

Checklist

stratoula commented Feb 8, 2021

elizabetdev commented Feb 9, 2021

stratoula commented Feb 9, 2021

nickofthyme commented Feb 9, 2021

stratoula commented Feb 11, 2021

stratoula commented Feb 11, 2021 • edited Loading

nickofthyme commented Feb 11, 2021

stratoula commented Feb 15, 2021

markov00 commented Feb 16, 2021

stratoula commented Feb 18, 2021

MichaelMarcialis commented Feb 18, 2021

stratoula commented Feb 22, 2021

stratoula commented Feb 22, 2021

stratoula commented Feb 25, 2021

stratoula commented Feb 25, 2021

stratoula commented Mar 1, 2021

ThomThomson left a comment

Choose a reason for hiding this comment

myasonik left a comment

Choose a reason for hiding this comment

myasonik Mar 8, 2021

Choose a reason for hiding this comment

stratoula commented Mar 9, 2021

stratoula commented Mar 9, 2021

myasonik left a comment

Choose a reason for hiding this comment

stratoula commented Mar 10, 2021

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Mar 11, 2021

Choose a reason for hiding this comment

myasonik Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

nickofthyme Mar 11, 2021

Choose a reason for hiding this comment

stratoula Mar 12, 2021

Choose a reason for hiding this comment

nickofthyme Mar 15, 2021

Choose a reason for hiding this comment

stratoula Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

myasonik left a comment

Choose a reason for hiding this comment

stratoula commented Mar 12, 2021

stratoula commented Mar 15, 2021

stratoula commented Mar 16, 2021

kibanamachine commented Mar 16, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

myasonik left a comment

Choose a reason for hiding this comment

stratoula commented Feb 8, 2021 •

edited

Loading

stratoula commented Feb 11, 2021 •

edited

Loading

myasonik Mar 11, 2021 •

edited

Loading

stratoula Mar 16, 2021 •

edited

Loading