elastic / eui

Elastic UI Framework 🙌
https://eui.elastic.co/
Other
58 stars 842 forks source link

[EuiColorPaletteDisplay] Different representation of palette color scheme when in fixed mode #4664

Closed dej611 closed 11 months ago

dej611 commented 3 years ago

I've noticed that in fixed mode the EuiColorPaletteDisplay editor has a left-shifted representation of a palette, when compared to the one from the EuiColorStops.

This happens with the same data provided to both, as reproduced here: https://codesandbox.io/s/affectionate-dew-chmnw?fontsize=14&hidenavigation=1&theme=dark

Screenshot:

Screenshot 2021-03-25 at 13 07 15

We're using them both in Lens as to show and edit a palette and it may be confusing the configure one thing watching it one way, then close the panel and finding a different representation

dej611 commented 3 years ago

I've spent some time trying to figure out a possible solution for this problem, and I concluded there's no right universal solution between the two components.

Probably changing the EuiColorPaletteDisplay stops order may be the solution, but it may have some impacts on current code, as the same format is used also for other components like EuiColorPalettePicker. The term stop as in the EuiColorPaletteDisplay or EuiColorPalettePicker has more a meaning of "segment relative width", while in the EuiColorStop is intended to indicate a precise point to start a new segment (close to the former, but not quite).

Converting the EuiColorStop to use the same semantic as the other components, applying an internal left shift to the input palette to work, while exposing the other semantic on change, is not feasible.

In few scenarios like the one above, where the purple stop is set at 100%, after the shift the information would be lost:

Screenshot 2021-03-25 at 13 07 15

Another exception would be the handling of a single stop palette:

Screenshot 2021-04-02 at 16 49 31

As for the moment the easiest workaround I found is the use the EuiColorStop format for the palette and apply a right shift to the ColorStop list when it is required to be displayed:

function shiftPalette(stops: ColorStop[]) {
  // shift everything right and add an additional stop at the end
  const result = stops.map((entry, i, array) => ({
    ...entry,
    stop: i + 1 < array.length ? array[i + 1].stop : 100,
  }));
  if (stops[stops.length - 1].stop === 100) {
    // pop out the last value (to avoid any conflict)
    result.pop();
  }
  return result;
}

The function above is applied only to inputs that come from a EuiColorStop component, which is always marked as custom palette in our case.

elizabetdev commented 3 years ago

Thanks, @dej611 for trying to find a solution.

@thompsongl is there a solution you might think of to make these two components work well together?

It seems that the EuiColorPalettePicker and EuiColorPaletteDisplay need to be improved to handle this scenario. The EuiColorPalettePicker uses the EuiColorPaletteDisplay internally. So we just need to focus more on the EuiColorPaletteDisplay.

By talking with @dej611, I tried to figure out what we need to improve in EuiColorPaletteDisplay to make it work better with the EuiColorStops. And these are the requirements we think it might work:

Also, the EuiColorPaletteDisplay right now has a min of 0. What happens if we want a different min? And it doesn't allow to set a max. Should we allow to set a min and max?

thompsongl commented 3 years ago

It's probably worthwhile to recount where the concept for EuiColorStops started.

image

Its purpose was/is to replace the custom color ramp UI in the maps application and the color stops API reflects this. In the screenshot, you notice that the number value for the first stop is not editable. The idea is that the first color starts at 0 (or the data range minimum) and stops if/when the next color is defined. So, EuiColorStops color stops work by defining the start value and that color will go to infinity (or the range max) if no other colors are defined.

When we created EuiColorPaletteDisplay, I don't recall discussing whether the APIs should match, but I think that we should at least introduce the option to make them match.

Have a transparent color (or representation) in EuiColorPaletteDisplay when the first available stop is > 0%

Transparent could work. I'd look to your design recommendation for this @miukimiu

Maybe a prop could be passed to the EuiColorPaletteDisplay saying I want this component to behave like the EuiColorStop

Yes, and we can then decide if that should be the default and introduce a breaking change. The difference between the two modes would only affect the "with stops" versions of EuiColorPaletteDisplay and indicate whether the color stop values are the start or stop values for the colors.

Also, the EuiColorPaletteDisplay right now has a min of 0

We will need min and max props to define the range for the "behave like EuiColorStops" case.

cee-chen commented 11 months ago

Closing this as not planned as EuiColorStops has been deprecated & removed. If there's still a feature request here for EuiColorPaletteDisplay, feel free to shout/correct me and I'll re-open!