[FEATURE] Ability to edit thresholds in panel options for gauge chart #968

eunicorn · 2023-02-17T20:05:16Z

CHANGES

You can now define default thresholds colors and add color palette in charts theme.
You can add, update, and delete thresholds
Implemented a color picker using react-colorful
Added percentage mode

TO DO

tests
add thresholds editor to other charts
add support for min ?

Signed-off-by: Eunice Wong <eunicewong@live.com>

eunicorn · 2023-02-17T20:14:56Z

ui/components/package.json

@@ -43,6 +43,7 @@
    "lodash-es": "^4.17.21",
    "mathjs": "^10.6.4",
    "mdi-material-ui": "^7.4.0",
+    "react-colorful": "^5.6.1",


I was choosing between either react-color or react-colorful, the two most popular color picker libraries.

I ended up choosing react-colorful because the design is simple (@foxbat07 approved), package is smaller, and it is built with react hooks and functional components. It also ended up being very easy to use. But definitely feel free to let me know if you think otherwise/have a strong opinion which color picker to use!

eunicorn · 2023-02-17T20:19:25Z

ui/panels-plugin/src/model/thresholds.ts

 export type GaugeColorStop = [number, string];

 export type EChartsAxisLineColors = GaugeColorStop[];

-export interface StepOptions {


moved StepOptions and ThresholdOptions to plugin-system since I needed these types in ThresholdsEditor which lives in plugin-system/components

Another option could be to move ThresholdsEditor to @perses-dev/components and the types to @perses-dev/core.

We definitely need to decide on long term rules for what goes in each package since currently it isn't consistent (see discussion here: LegendOptionsEditor, UnitSelector, unit types are in @perses-dev/components for instance). Probably fine to wait until @juliepagano looks into package reorganization though too

I think ThresholdsEditor being in @perses-dev/components will also make it easier to have context-aware components in the future (ex: TimeSeriesChart proposal). If it lives in plugin-system, then ColorPicker.tsx would need to move to avoid a circular dependency for instance

i think i was debating to move it to @perses-dev/components but got confused since some components like CalculationSelector are in plugin-system. Can't wait to get this figure out lol.

Anyways moved ThresholdsEditor to @perses-dev/component 😄

eunicorn · 2023-02-17T20:22:37Z

ui/panels-plugin/src/model/thresholds.ts

  const defaultThresholdSteps: EChartsAxisLineColors = [[0, defaultThresholdColor]];

  if (thresholds.steps !== undefined) {
    // https://echarts.apache.org/en/option.html#series-gauge.axisLine.lineStyle.color
    // color segments must be decimal between 0 and 1
    const segmentMax = 1;
    const valuesArr: number[] = thresholds.steps.map((step: StepOptions) => {
+      if (thresholds.mode === 'percentage') {
+        return step.value / 100;


we will have to update this once we support min

ui/plugin-system/src/model/thresholds.ts

Signed-off-by: Eunice Wong <eunicewong@live.com>

schemas/common/threshold.cue

sjcobb · 2023-02-21T17:00:03Z

ui/panels-plugin/src/model/thresholds.ts

-  RED: 'rgba(234, 71, 71, 1)', // red.500
-};
-
-export const ThresholdColorsPalette = [ThresholdColors.ORANGE, ThresholdColors.RED];


I really like that threshold colors moved to the chartsTheme, but one difference I'm seeing is previously orange and red were the defaults if a step color wasn't set explicitly, now it shows as blue for ex:

talked to @foxbat07 about this and decided to follow the mui alert colors (info, warning, error)

thresholds: { defaultColor: muiTheme.palette.success.main, palette: [muiTheme.palette.info.main, muiTheme.palette.warning.main, muiTheme.palette.error.main], }

Signed-off-by: Eunice Wong <eunicewong@live.com>

cndonovan

This is so nice, yay for our users

cndonovan · 2023-02-22T00:32:28Z

ui/components/src/ColorPicker/ColorPicker.tsx

+    const inputValue = e.target.value.replace(/([^0-9A-F]+)/gi, '').substring(0, 8);
+    setValue(`#${inputValue}`);
+    // only set color if input value is a valid hex color
+    if (isValidHex(e.target.value)) {


I see there's a difference in usage between color and value -- if you wanted, you could leave a comment above the useState lines to explain the difference between the two variables

I'm reading as:
color stores the actual input from the user, but only if it's valid hex format
value stores the hex value we want to use, including the #, and does some sanitizing of the user input -- is that right?

Oh maybe it's that value is meant to be the visible value for the controlled text input, and color is what we'll pass around internally to use in other parts of the code -- ok I think I'm following now

you're correct - i'll add comments in the code to make this more clear :)

cndonovan · 2023-02-22T00:41:35Z

ui/components/src/ColorPicker/ColorPicker.tsx

+  );
+};
+
+const isValidHex = (value: string, alpha?: boolean): boolean => {


Does the color-picker library come with a function that does this?

not yet but i actually got this function from the library. It currently only exports an input component with the validation logic, but there is an open issue to support custom input components omgovich/react-colorful#157

cndonovan · 2023-02-22T00:45:44Z

ui/components/src/ColorPicker/ColorPicker.tsx

+        fullWidth
+        value={value}
+        onChange={handleInputChange}
+      />


food for thought: when I'm entering colors, I sometimes don't know if I'm supposed to use rgb format (e.g. (24, 32, 150)) or hex format.

ideas:

We could add a label that says "Hex value" or something like

We could have a placeholder value in hex format

not a big deal though, just ideas

right now it always prefixes the input with a pound sign to indicate that it's hex format and i don't think it should let you enter parentheses and commas

oo yes the # prefix does exactly that!

cndonovan · 2023-02-22T00:46:34Z

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

+    <Stack flex={1} direction="row" alignItems="center" justifyContent="space-between" spacing={1}>
+      <ThresholdColorPicker label={label} color={color} onColorChange={onColorChange} />
+      <FormLabel htmlFor={label}>{label}</FormLabel>
+      <TextField id={label} inputRef={inputRef} type="number" value={value} onChange={onChange} onBlur={onBlur} />


Yessss love using the type="number" (those little up and down arrows!)

ui/components/src/ThresholdsEditor/ThresholdColorPicker.tsx

cndonovan · 2023-02-22T01:08:56Z

ui/components/src/ThresholdsEditor/ThresholdColorPicker.tsx

+  iconColor?: string;
+  isSelected?: boolean;
+}>(({ iconColor, isSelected }) => ({
+  backgroundColor: isSelected && iconColor ? `${iconColor}3F` : 'undefined', // 3F represents 25% opacity


cndonovan · 2023-02-22T01:19:21Z

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

+  const [steps, setSteps] = useState(thresholds?.steps);
+  useEffect(() => {
+    setSteps(thresholds?.steps);
+  }, [thresholds?.steps]);


Noticed that we're using palette in other places (vs steps). Is palette like a simplified version of the same data in steps?

palette is the color palette we'll use in the chart and steps are the actual values.

for example, if steps[0] is 10 and palette[0] is yellow, we'll show yellow for values greater than or equal to 10

cndonovan · 2023-02-22T01:27:22Z

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

+    if (!recentlyAddedInputRef.current || !focusRef.current) return;
+    recentlyAddedInputRef.current?.focus();
+    focusRef.current = false;
+  }, [steps?.length]);


Might not be possible, but is there a way to call .focus() from inside the addThresholdChange handler and get it to work?

Hmm i don't think so? We don't have a ref to the text input yet since it hasn't been rendered until steps is updated

cndonovan · 2023-02-22T01:34:49Z

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

+            const color = palette[steps.length] ?? getRandomColor(); // we will assign color from the palette first, then generate random color
+            steps.push({ color, value: (lastStep?.value ?? 0) + 10 });
+          } else if (steps) {
+            steps.push({ value: 10 });


Is this saying that any newly added threshold defaults to a value of 10?

if there is no previous threshold set (steps.length === 0), then the first threshold you add will default to 10

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

Signed-off-by: Eunice Wong <eunicewong@live.com>

juliepagano · 2023-02-22T17:28:13Z

UX feedback for a potential follow-up with @foxbat07. I find the top bar that communicates the ranges a little overwhelming to process quickly in some cases (e.g. it's a lot when there's a bunch of gauges right next to each other) because there's so many colors. I wonder if it might be helpful to add an option where you can disable them if you like?

juliepagano · 2023-02-22T18:04:40Z

I noticed that the button toggle highlights the "%" when it's selected...

...but not the "#." Is this a bug?

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

juliepagano · 2023-02-22T18:29:06Z

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

+      onChange(updatedThresholds);
+    }
+  };
+


Optional suggestion -- You've got a decent amount of logic around adjusting these values that's probably a bit of a pain to test in the context of a react component. If you pulled the core logic out into a few utility functions that you call from the component, you could more easily write some unit tests on those functions outside the react context. That said, you've already written a bunch of tests for the components, so may not be worth refactoring now.

oh nice i'll keep that in mind for next time, and writing those tests in react context actually helped me uncover a few accessibility issues

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx

juliepagano · 2023-02-22T18:32:40Z

If you have the time, I think this would be a good case to add some additional visual tests for the gauge chart. Let me know if you want help doing so.

Signed-off-by: Eunice Wong <eunicewong@live.com>

eunicorn · 2023-02-22T19:00:38Z

If you have the time, I think this would be a good case to add some additional visual tests for the gauge chart. Let me know if you want help doing so.

yes definitely! i am hoping to do this as one of the follow ups since this PR is already so big

juliepagano

LGTM! 🚀

juliepagano · 2023-02-22T19:23:53Z

FYI, in case you haven't done it before, you can remove the ❌ next to "happo" by clicking "details" and selecting "approve" from the review dropdown.

ui/components/src/OptionsEditorLayout/OptionsEditorGroup.tsx

saminzadeh · 2023-02-22T22:19:24Z

ui/components/src/ColorPicker/ColorPicker.tsx

+
+interface ColorPickerProps {
+  initialColor?: string;
+  onColorChange?: (color: string) => void;


What is the purpose of onColorChange? Seems like initialColor means that component is uncontrolled so I wonder if this can be misleading. Otherwise, it might make sense to use the controlled component paradigm with value and onChange

good call, replaced initialColor with color and onColorChange to onChange

Signed-off-by: Eunice Wong <eunicewong@live.com>

…hreshold-gauge Signed-off-by: Eunice Wong <eunicewong@live.com>

Signed-off-by: Eunice Wong <eunicewong@live.com>

sjcobb

🚀

eunicorn added 2 commits February 17, 2023 15:08

add thresholds to chart theme

f6c8cae

Signed-off-by: Eunice Wong <eunicewong@live.com>

implemented adding/delete threshold

07ae4ac

Signed-off-by: Eunice Wong <eunicewong@live.com>

eunicorn force-pushed the eunicorn/threshold-gauge branch from 7edd74c to fe05375 Compare February 17, 2023 20:09

eunicorn commented Feb 17, 2023

View reviewed changes

ui/plugin-system/src/model/thresholds.ts Outdated Show resolved Hide resolved

eunicorn added 2 commits February 17, 2023 15:48

implemented color picker

335384c

Signed-off-by: Eunice Wong <eunicewong@live.com>

implemented thresholds mode

11a66ed

Signed-off-by: Eunice Wong <eunicewong@live.com>

eunicorn force-pushed the eunicorn/threshold-gauge branch from a7726dd to 11a66ed Compare February 17, 2023 20:48

eunicorn requested review from a team, saminzadeh and sjcobb February 17, 2023 20:50

eunicorn marked this pull request as ready for review February 17, 2023 20:50

eunicorn added 2 commits February 17, 2023 16:08

format code

0085dc2

Signed-off-by: Eunice Wong <eunicewong@live.com>

added unit tests for thresholdsEditor

b6745e4

Signed-off-by: Eunice Wong <eunicewong@live.com>

eunicorn force-pushed the eunicorn/threshold-gauge branch from 49f101f to b6745e4 Compare February 18, 2023 00:19

fix lint error

873cda6

Signed-off-by: Eunice Wong <eunicewong@live.com>

sjcobb reviewed Feb 21, 2023

View reviewed changes

schemas/common/threshold.cue Outdated Show resolved Hide resolved

sjcobb reviewed Feb 21, 2023

View reviewed changes

eunicorn added 2 commits February 21, 2023 18:44

move thresholdEditor to components

dc95312

Signed-off-by: Eunice Wong <eunicewong@live.com>

update cue

6d552d8

Signed-off-by: Eunice Wong <eunicewong@live.com>

eunicorn requested a review from juliepagano February 21, 2023 23:50

cndonovan reviewed Feb 22, 2023

View reviewed changes

address pr comments

9ad09cd

Signed-off-by: Eunice Wong <eunicewong@live.com>

juliepagano reviewed Feb 22, 2023

View reviewed changes

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx Outdated Show resolved Hide resolved

juliepagano reviewed Feb 22, 2023

View reviewed changes

ui/components/src/ThresholdsEditor/ThresholdsEditor.tsx Show resolved Hide resolved

fix selected absolute state and move thresholdInput to its own file

b8ace1c

Signed-off-by: Eunice Wong <eunicewong@live.com>

juliepagano approved these changes Feb 22, 2023

View reviewed changes

saminzadeh reviewed Feb 22, 2023

View reviewed changes

update color picker

d20a41a

Signed-off-by: Eunice Wong <eunicewong@live.com>

eunicorn force-pushed the eunicorn/threshold-gauge branch from 348c92e to d20a41a Compare February 23, 2023 04:55

eunicorn added 4 commits February 27, 2023 14:36

switch back to yellow in thresholds color palette

634ac1f

Signed-off-by: Eunice Wong <eunicewong@live.com>

Merge branch 'main' of ssh://github.com/perses/perses into eunicorn/t…

6812c32

…hreshold-gauge Signed-off-by: Eunice Wong <eunicewong@live.com>

fix package-lock

dd53278

Signed-off-by: Eunice Wong <eunicewong@live.com>

update snapshot

2bf5bff

Signed-off-by: Eunice Wong <eunicewong@live.com>

sjcobb approved these changes Feb 27, 2023

View reviewed changes

eunicorn merged commit f61f37e into main Feb 27, 2023

eunicorn deleted the eunicorn/threshold-gauge branch February 27, 2023 23:37

[FEATURE] Ability to edit thresholds in panel options for gauge chart #968

[FEATURE] Ability to edit thresholds in panel options for gauge chart #968

Conversation

eunicorn commented Feb 17, 2023 • edited Loading

eunicorn Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjcobb Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunicorn Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

cndonovan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eunicorn Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

juliepagano commented Feb 22, 2023

juliepagano commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliepagano commented Feb 22, 2023

eunicorn commented Feb 22, 2023

juliepagano left a comment

Choose a reason for hiding this comment

juliepagano commented Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

eunicorn Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

sjcobb left a comment

Choose a reason for hiding this comment

eunicorn commented Feb 17, 2023 •

edited

Loading

eunicorn Feb 17, 2023 •

edited

Loading

sjcobb Feb 21, 2023 •

edited

Loading

eunicorn Feb 21, 2023 •

edited

Loading

eunicorn Feb 22, 2023 •

edited

Loading

juliepagano commented Feb 22, 2023 •

edited

Loading

eunicorn Feb 23, 2023 •

edited

Loading