Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Lens] Color Mapping style Enhancements #167703

Closed
19 of 27 tasks
Tracked by #167506
MichaelMarcialis opened this issue Sep 29, 2023 · 13 comments
Closed
19 of 27 tasks
Tracked by #167506

[Lens] Color Mapping style Enhancements #167703

MichaelMarcialis opened this issue Sep 29, 2023 · 13 comments
Labels
enhancement New value added to drive a business result Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Sep 29, 2023

Let me begin by giving you proper praise for your beautiful work on the Lens color mapping PR, @markov00! It was a real delight to work with both you and @gvnmagni on this feature. Seeing it come to life has been nothing short of amazing.

Per your request, I've reviewed the latest updates and compiled a list of update comments for you below. As always, let me know if you have any questions.

Responses to previous comments

Should the new "Use new color mapping (tech preview)" switch be scoped and relocated from the individual dimension item level to a larger visualization level? By scoping this setting to individual dimension items, it may require users with multiple color-oriented dimension to change the setting in multiple locations. Worse still, it opens up the possibility for a single visualization using the new color mapping feature in some dimensions and the legacy color selection in others. This may make it more difficult for us to avoid breaking changes in the future and may also confuse users as to why they are seeing two different color interfaces within the same visualization. Thoughts? CCing @timductive and @ninoslavmiskovic.

Answer: you are right about this, but the idea here was to make it prominent and quickly available for the switch. It could generate confusion, but I believe is fine now for this tech preview. For every new chart this option will be already on ON so only when the hard case of 2 layers with color mapping this could cause confusions. Waiting for @timductive @ninoslavmiskovic opinions.

Including this here so we can get input from @ninoslavmiskovic and @timductive.

Are we confident that the term "Sequential" will resonate more with users than something like "Gradient"? To my mind, "Gradient" seems easier to understand, but I'm curious to know what others think.

Answer gradient is about the color, and sequential is about the scale. I believe in educating the user to the right words even if these don't resonate well because they can then build up a vocabulary that can be reused in other software.

  • Fair enough. It may be worth mentioning that we do use the term "gradient" elsewhere in this interface though (such as the "Invert gradient" button). Are we comfortable using multiple terms for what may ultimately feel like the same thing to users (even though they are technically different)?

Change the popover's section titles from 14px to 12px ().

  • It looks like this feedback was applied to the color picker popover's tab text, rather than the section titles (i.e. "Palette colors" and "Neutral colors"). Please forgive me, I should have been clearer here. Would it be possible to move the <EuiTitle size="xxxs"> components from the tabs to the titles below?

Change the "Contrast-aware greys" section title to "Neutral colors" and append it with an EuiIconTip explaining that "The provided neutral colors are theme-aware and will change appropriately when switching between light and dark themes."

  • Thanks for making this change. It looks like the period at the end of the tooltip sentence got missed. Would it be possible to add?

Can we have the buttons for adding color stops to the gradient only show only when the user is hovering/focusing the gradient interface (rather then having them be an ever-present element covering the gradient track)? Perhaps we could also add a subtle 0–100% opacity transition as well?

NOTE: I tried but the only thing I can do is to highlight the + button when over the + button, not from the line gradient. We can double check together on how to fix that.

Certainly, I'd be happy to help with this one. Feel free to pop some time on the calendar if you want to do some CSS pairing.

  • In the mean time, could we update the styles for the the add color stop buttons to:
    • Use euiTheme.colors.emptyShade background color.
    • Use euiTheme.colors.subduedText icon color.
    • Change to euiTheme.colors.text icon color on hover/focus.

CleanShot 2023-10-02 at 13 47 46

The "Assignments" footer buttons for "Add assignment" and "Invert gradient" should use the flush="both" prop to better align to the left edge of the preceding contents. Note that you'll need to add some space between the buttons to compensate for the removal of their padding.

  • Thanks for addressing this. Can we increase the gap between the footer buttons from 8px to 16px?

When using the "Date histogram" function, editing the color mapping shows the value assignments in what appears to be Unix timestamp format. Can we change this to a more human-readable format (ideally the same format used in the visualization legend)?

NOTE coloring by "date" is not recommended. I can't match partial date written as string to timestamps used in the code also because I don't have a way to distinguish between a date written by the user and different type of category, I can parse only text input. If you need to specify date then we need date pickers on every category that is something too complex for an unused case. Let's collect feedback and see how it works. (also consider the fact that the date histogram could be "auto" computed, meaning that what you describe as date could change depending on the time window. Probably is better to use a color by range instead.

  • Using the color-by-range pattern makes sense to me at initial glance, though I'd have to think about it longer to see if there are any sharp corners. In any case, let's add this as a discussion point when we get around to updating the color-by-range interfaces.

If I delete a dimension item that has a custom color configuration (i.e. user has turned off auto-assign and applied a custom set of terms/colors), adding a new dimension item restores that previously deleted dimension item's custom color configuration. Would it be better to start from scratch and apply the default presets (with auto-assign enabled)?

Note Still checking this

  • Including this in case you had any updates.

The same term can be assigned to multiple colors in assignments. This shouldn't be allowed. Even more confusing is that rather than the last assignment being the one that is honored (via a perceived cascade effect), it seems the first assignment is the one that is honored. Instead, can we simply not allow the same term to be assigned to more than one color? We can achieve this by removing the previous term assignment when the user attempts to assign a term in use to a different color.

NOTE Is a cascade effect actually, the first assigned the only one applied. I've added a warning sign on each duplicate category so the user can decide what to do and which to remove

  • I think the warning approach you described will work nicely for the short-term. Would it be worth considering the automatic swapping of terms that I mentioned above as a possible future enhancement?

New comments

Root-level configuration flyout

  • Change the root-level flyout's color mapping edit icon button to use color="text".

  • Attempting to click the top or bottom portions above or below the color mapping preview strip doesn't open the second-level color mapping flyout, despite showing a pointer icon on hover. Only clicking directly on the preview strip does it function correctly. Can we correct this so the pointer is only shown on areas that will trigger the second-level flyout?

    CleanShot 2023-10-02 at 13 49 47

  • Can we add a tooltip to edit icon button that says "Edit color mapping"?

Color configuration flyout

Opt-in switch

  • For the new color mapping switch, would you be open to the idea of moving it out of the flyout's main content and relocating it to the flyout header or footer? I think moving it to the header makes sense hierarchically. Though the footer helps not make it the most prominent element in the flyout. In either case, it helps visually separate the switch from the main color configuration form. Below are some quick mockups of these concepts. Thoughts?

    Header example Footer example
    CleanShot 2023-10-02 at 13 34 41 CleanShot 2023-10-02 at 13 29 53
  • Change the "Use the new Color Mapping feature" text to "Try new color features".

  • Are we allowed to abbreviate "Technical preview" to "Tech preview" (CCing @amyjtechwriter)? If not, yet we still wish to be more succinct than to spell it out fully, perhaps a icon-only badge or beta badge (with accompanying tooltip) would be most appropriate?

Scale button group

  • The horizontal space between this button group and the preceding "Color palette" selector is currently 24px. Can we reduce this to 16px?

  • It appears that the currently selected button's icon still doesn't show a white fill color. Are we using the new icons added to EUI? Or are these still the custom icons? If custom, can we switch the EUI versions (assuming EUI has been updated in Kibana)?

    CleanShot 2023-10-02 at 13 52 03

Mapping assignments

General

  • The space between "Mapping assignments" and the preceding "Color palette"/"Scale" rows is currently 8px. Could we change this to 16px to better match EUI form row spacing?

  • The space between the "Mapping assignments" label (and "Auto assign" switch) and the subsequent gray container is currently 8px. Can we change this to 4px to better match EUI form label spacing?

  • The "Mapping assignment" label element doesn't currently indicate what element it applies to (ex. via the for attribute). Could we account for this as an accessibility enhancement (similar to what we do for other areas of Lens, like multi-field top values)?

  • Can we add a 6px border-radius (euiTheme.border.radius.medium) for the gray container housing the mapping assignments? Doing so would help to better match with similar interfaces in Lens.

  • The mapping assignment delete buttons aren't visually vertical centered with a single line combobox. I'm guessing this is due to the top-aligning of the contents for each mapping row that we discussed for the multiline combobox situations. Would it be possible to give these delete buttons the impression of being vertically centered on in those single line combobox instances by pushing the delete buttons down a few pixels (4px)?

  • Please forgive this tiny nitpick, but could we adjust the dividing border above "Unassigned terms" to touch the horizontal bleeding edges of it's gray container?

  • Semantic question: should the "Unassigned terms" input be read-only (rather than disabled)? I'm going back and forth in my head, but leaning towards read-only as this particular input never has the possibility of being enabled or having its value changed. Thoughts? CCing @1Copenut as well, in case he has any thoughts from the accessibility side.

  • If a sequential color scale is in use, could we add some space to the left of the "Unassigned terms" color swatch to offset it enough to account for the gradient slider and bring it back in alignment with the color swatches that appear above it?

Color picker popover

  • Under the color picker popover's section titles ("Palette colors" and "Neutral colors"), can we add 8px of spacing between them and their subsequent color swatches?

  • Let's change the color picker popover's anchorPosition to be downLeft. Doing so will prevent the popover contents from jumping around when transitioning between the "Colors" and "Custom" tabs, or when the color contrast warning appears (assuming the viewport has space to do so).

  • For the hex value input in the color picker popover, if the user clears the input or supplies an invalid value, we immediately hit them with an error message and text. Would it be better (and less stressful for users) to not show the input in error and instead just restore the last valid value on blur of the input? We appear to already be doing just that on close of the popover anyway.

@MichaelMarcialis MichaelMarcialis added enhancement New value added to drive a business result Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens labels Sep 29, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@stratoula stratoula added the impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. label Oct 2, 2023
@markov00
Copy link
Member

markov00 commented Oct 2, 2023

@MichaelMarcialis

The horizontal space between this button group and the preceding "Color palette" selector is currently 24px. Can we reduce this to 16px?

Just to be sure: 24px is the default (large) gap of our flexbox, if I choose medium then it goes to 12px. There is no intermediate 16px as gap size for eui. I can just change it manually in the style but I just like to confirm that with you.

For the new color mapping switch, would you be open to the idea of moving it out of the flyout's main content and relocating it to the flyout header or footer? I think it makes sense to do so hierarchically and would also help divide it from the form elements that actually change the color mapping. I'll attempt to mock this up shortly.

I'm fine with that, that was my first idea initially. can you please provide an example on how it should looks like?

Semantic question: should the "Unassigned terms" input be read-only (rather than disabled)? I'm going back and forth in my head, but leaning towards read-only as this particular input never has the possibility of being enabled or having its value changed. Thoughts? CCing @1Copenut as well, in case he has any thoughts from the accessibility side.

Could you please also give me an idea on how it should look like, because read-only looks like this today:
Screenshot 2023-10-02 at 10 13 25

Let's change the color picker popover's anchorPosition to be downLeft. Doing so will prevent the popover contents from jumping around when transitioning between the "Colors" and "Custom" tabs, or when the color contrast warning appears (assuming the viewport has space to do so).

The problem here is that we changed that to upLeft because it has by default more space and the tooltip is not repositioned based on its content when you switch from palette to custom. This can cause the tooltip not to be accessible if it opens to the bottom but then you switch to custom color. see @drewdaemon comment here #162389 (review)

I think the warning approach you described will work nicely for the short-term. Would it be worth considering the automatic swapping of terms that I mentioned above as a possible future enhancement?

I personally think that automatic swapping is not a good practice, mainly because we don't have a nice way to animate/show what is going on. You just swap things in the background and the user possibly not even notice that.
I believe we can come out with a good tradeoff without tricking the user with background unnoticeable behaviors.

It looks like this feedback was applied to the color picker popover's tab text, rather than the section titles (i.e. "Palette colors" and "Neutral colors"). Please forgive me, I should have been clearer here. Would it be possible to move the components from the tabs to the titles below?

This is what happen if I change the title to be the previous 14px and the section titles to be 12px, are you fine with that?
Screenshot 2023-10-02 at 15 40 32

@MichaelMarcialis
Copy link
Contributor Author

MichaelMarcialis commented Oct 2, 2023

The horizontal space between this button group and the preceding "Color palette" selector is currently 24px. Can we reduce this to 16px?

Just to be sure: 24px is the default (large) gap of our flexbox, if I choose medium then it goes to 12px. There is no intermediate 16px as gap size for eui. I can just change it manually in the style but I just like to confirm that with you.

Great catch, @markov00! This is actually an EUI bug. Looks like when they switched from using margins to the gap property in EuiFlexGroup, they inadvertently changed the gutterSize="m" prop from being 16px to 12px. I'll create an EUI issue for that. For now, let's go ahead and use gutterSize="m" to reduce the gap, and it'll inherit the proper size when the fix is applied in EUI.

For the new color mapping switch, would you be open to the idea of moving it out of the flyout's main content and relocating it to the flyout header or footer? I think it makes sense to do so hierarchically and would also help divide it from the form elements that actually change the color mapping. I'll attempt to mock this up shortly.

I'm fine with that, that was my first idea initially. can you please provide an example on how it should looks like?

I've updated my original comments above to include a quick mockup of these possible directions.

Semantic question: should the "Unassigned terms" input be read-only (rather than disabled)? I'm going back and forth in my head, but leaning towards read-only as this particular input never has the possibility of being enabled or having its value changed. Thoughts? CCing @1Copenut as well, in case he has any thoughts from the accessibility side.

Could you please also give me an idea on how it should look like, because read-only looks like this today:

Yeah, the read-only styling for form components like EuiFieldText could probably use some love. In taking a second look at them, they don't appear to have a focus state (which they probably should, as read-only elements are focusable). Also, it would probably be nice if they shared the same background color as disabled form inputs (as the current white background makes them nearly indistinguishable from normal form inputs).

@1Copenut, assuming you agree this form element should be read-only instead of disabled, do you also agree with the styling comments above? If so, I can write up an EUI issue for this. @markov00, in the mean time, we can continue to use disabled until improvements are made to the read-only styling.

Let's change the color picker popover's anchorPosition to be downLeft. Doing so will prevent the popover contents from jumping around when transitioning between the "Colors" and "Custom" tabs, or when the color contrast warning appears (assuming the viewport has space to do so).

The problem here is that we changed that to upLeft because it has by default more space and the tooltip is not repositioned based on its content when you switch from palette to custom. This can cause the tooltip not to be accessible if it opens to the bottom but then you switch to custom color. see @drewdaemon comment here #162389 (review)

Ah, that makes sense. If it's not possible for us to dynamically position the popover based on the tallest content (either "Colors" or "Custom" tab), then we can keep as is.

I think the warning approach you described will work nicely for the short-term. Would it be worth considering the automatic swapping of terms that I mentioned above as a possible future enhancement?

I personally think that automatic swapping is not a good practice, mainly because we don't have a nice way to animate/show what is going on. You just swap things in the background and the user possibly not even notice that. I believe we can come out with a good tradeoff without tricking the user with background unnoticeable behaviors.

I appreciate that perspective. In this particular case though, I don't believe such behavior would be perceived as our tricking the user. I believe it honors the user's request in the more expedient manner. In this scenario, the user is requesting that we assign a term to a color. If that term is already in use, the user either 1) isn't aware that this term is already being assigned to another color, or 2) knows that it is assigned to another color but wishes to change it. In both cases, automatically swapping the term out of it's old assignment and into the new one makes sense and benefits the user. That said, I wouldn't be opposed to bolstering that experience with some animation to make it even more obvious. Thoughts?

This is what happen if I change the title to be the previous 14px and the section titles to be 12px, are you fine with that?

Yes, this (in addition to 8px of margin below the titles) is exactly what I had in mind. Thanks!

@timductive timductive added loe:medium Medium Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. and removed impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. labels Oct 4, 2023
@1Copenut
Copy link
Contributor

1Copenut commented Oct 4, 2023

Semantic question: should the "Unassigned terms" input be read-only (rather than disabled)? I'm going back and forth in my head, but leaning towards read-only as this particular input never has the possibility of being enabled or having its value changed. Thoughts? CCing @1Copenut as well, in case he has any thoughts from the accessibility side.

Yes, I agree this would be better served as read-only instead of disabled. Read-only lets screen readers hook into the input text and announce it instead of hiding the input as a tab stop. Screen reader users can hear the disabled input in the forms menu and if they're traversing with arrow keys, but a lot of folks use the Tab key and might miss that information.

Yeah, the read-only styling for form components like EuiFieldText could probably use some love. In taking a second look at them, they don't appear to have a focus state (which they probably should, as read-only elements are focusable). Also, it would probably be nice if they shared the same background color as disabled form inputs (as the current white background makes them nearly indistinguishable from normal form inputs).

Agreed, input[read-only] elements should have a focus state and something that clearly delineates them as focusable but not editable. I know that's a tall order to accomplish visually; semantic devices announce the input as "Your text here, read only" and we want to communicate this text can be copied, not edited, but is not disabled.

@mbarretta
Copy link

Lots of related history that I don't have the capacity to read through and grok, so this might be retread of a previous discussion.

I've talked with folks who have a few different analytic tools seeking consistency among color schemes and maps. The one provider I've heard a few times is HoloViz Colorcet.

The ask would be for some means for us to leverage 3rd party libraries within Lens and Maps.

@amyjtechwriter
Copy link
Contributor

Hi @MichaelMarcialis if "Technical preview" is too long, let's please go with the badge and tooltip option.

@markov00
Copy link
Member

The ask would be for some means for us to leverage 3rd party libraries within Lens and Maps.

@mbarretta I believe this can be achieved when we introduce palette editing/authoring. Categorical color mapping can be added quickly and easily, probably a bit more complex could be the gradient coloring that in some cases is not just a pure interpolation between colors but are specific functions with their own weights.

@mbarretta
Copy link

mbarretta commented Nov 29, 2023

I believe this can be achieved when we introduce palette editing/authoring.

@markov00 Nice! Is this the right issue to follow for that, or is there another one out there?

@markov00
Copy link
Member

@mbarretta I've created an issue here for that #172139

@markov00 markov00 changed the title [Lens] Color Mapping MVP Enhancements [Lens] Color Mapping style Enhancements Mar 25, 2024
@MichaelMarcialis
Copy link
Contributor Author

@teresaalvarezsoler: I've re-reviewed this list. Most of the items have already been addressed or are no longer valid. I've checked off those ones from the list. The small remainder of open issues and questions are as follows:

  • Should the "Use new color mapping feature" switch continue to be offered at the individual dimension item level, or should this be a visualization-wide setting?

  • Do we want to provide a special variant of the color mapping interface to better accommodate coloring by date? Currently when using the "Date histogram" function in a breakdown dimension, editing the color mapping shows the value assignments in Unix timestamp format, which is very difficult to work with.

    Image

  • Change the root-level flyout's color mapping edit icon button to use color="text".

    Image

  • Attempting to click the top or bottom portions above or below the color mapping preview strip doesn't open the second-level color mapping flyout, despite showing a pointer icon on hover. Only clicking directly on the preview strip does it function correctly. Can we correct this so the pointer is only shown on areas that will trigger the second-level flyout?

    CleanShot 2023-10-02 at 13 49 47

  • For the new color mapping switch, would you be open to the idea of moving it out of the flyout's main content and relocating it to the flyout header or footer? I think moving it to the header makes sense hierarchically. Though the footer helps not make it the most prominent element in the flyout. In either case, it helps visually separate the switch from the main color configuration form. Below are some quick mockups of these concepts. Thoughts?

    Header example Footer example
    CleanShot 2023-10-02 at 13 34 41 CleanShot 2023-10-02 at 13 29 53
  • Change the "Use the new Color Mapping feature" text to "Try new color features".

  • Consider using an icon-only beta badge (with accompanying tooltip) to mark the switch to new color features as "Technical preview".

  • Under the color picker popover's section titles ("Palette colors" and "Neutral colors"), can we add 8px of spacing between them and their subsequent color swatches?

    Image

@teresaalvarezsoler
Copy link

@MichaelMarcialis thanks.

Let's not do any of the items related to the switcher and the Technical Preview badge. I think we need to focus on how to make this GA and get rid of both things altogether.

Let's also not do the timestamps improvements until we hear any feedback from actual users. I don't think colouring timestamps is a common practice.

Attempting to click the top or bottom portions above or below the color mapping preview strip doesn't open the second-level color mapping flyout, despite showing a pointer icon on hover. Only clicking directly on the preview strip does it function correctly. Can we correct this so the pointer is only shown on areas that will trigger the second-level flyout?

I don't understand this issue. What is it about?

I think we should close this issue and open a new one that contains only the few remaining px adjustments so we can assign proper effort and impact in our next triage session.

@MichaelMarcialis
Copy link
Contributor Author

Attempting to click the top or bottom portions above or below the color mapping preview strip doesn't open the second-level color mapping flyout, despite showing a pointer icon on hover. Only clicking directly on the preview strip does it function correctly. Can we correct this so the pointer is only shown on areas that will trigger the second-level flyout?

I don't understand this issue. What is it about?

The areas indicated in red show a mouse pointer cursor on hover, but clicking doesn't actually open the color mapping interface. Will try to show in a GIF below.

Image

I think we should close this issue and open a new one that contains only the few remaining px adjustments so we can assign proper effort and impact in our next triage session.

Sounds good! Let me know if you want me to write it up or if you're already on it.

@teresaalvarezsoler
Copy link

Ah I see now. Thanks for clarifying. Can you better go ahead close this one and create the new issue please? I don't want to miss anything that you think it's necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Lens impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

No branches or pull requests

9 participants