refactor(xy): drop triple equals true on booleans #1350

wayneseymour · 2021-09-01T14:57:24Z

Summary

Refactor textContrast check to use truthy,
instead of using === true.

Details

Remove a little duplication,
while in the "neighbourhood".

Issues

Helps with: #1303

Addresses check for === true, === false, return true etc. redundancy candidates and eliminate

wayneseymour · 2021-09-01T15:01:36Z

Please feel free to close this, make fun of it, tell me it sucks, etc! hahahaaa @monfera

packages/charts/src/chart_types/xy_chart/utils/axis_utils.ts

monfera · 2021-09-01T17:59:09Z

packages/charts/src/common/color_calcs.ts

@@ -142,6 +142,12 @@ export function colorIsDark(color: Color): boolean {
  return luminance < 0.2;
 }

+const colors = (tr: any) => (tg: any) => (tb: any) => ({


Good idea to abstract out this util indeed! In this case there's no current or foreseeable partial application per color channel, so it could be a single parameter list, and the symbols can simplify to r, g, b as the t prefix carries no meaning in the scope of this function

Ah, that makes sense.

This function is basically a way to invert color channels.
There was also another, definitely simplifiable, piece of code:

? to === undefined ? `rgb(${255 - tr}, ${255 - tg}, ${255 - tb})` : `rgba(${255 - tr}, ${255 - tg}, ${255 - tb}, ${to})`

that just create an rgb or an rgba color string depending if the opacity is defined or not.

I think we can easily replace this by always rendering rgba strings (non rgbA 😉) where the opacity is always 1 if the param is undefined, something along those lines:

function invertColor(r:RGB, g:RGB, b:RGB, o = 1) { return `rgba(${255 - r}, ${255 -g},${255 - b},${o})`; }

Wow, that's waaaaaay more simple!

Full disclosure, I had no idea of the context of all this rgb "stuff" lol

Ok, I'm going to push what I have so far, but I'm also going to go back through and change it to have this invertColor fn. I really like the simplicity.

monfera · 2021-09-01T18:03:47Z

Hi @wayneseymour thanks for the contrib!

There's an eslint issue: we recently got rid of husky (blame me), so the code autoformatting on commit went away, we pretty much lean on our editors linking to the ever current eslint rules, not just for the red squigglies but also for reformatting on autosave:

The "Semantic Pull Request" check was red as it enforces commitizen rules, in this case,

one of refactor, test, chore, feat,
plus, if applicable, the area in braces, such as xy, partition, heatmap etc.

It's late and I haven't checked the actual logic, but it looks like at least the 3rd visual regression test may have picked up a logic change, worth checking, as here the outcomes are expected to remain the same. Glad to get you started on ways to run the VRT locally for a sometimes faster turnaround

wayneseymour · 2021-09-02T10:29:18Z

packages/charts/src/common/color_calcs.ts

@@ -155,27 +161,29 @@ export function getTextColorIfTextInvertible(
 ): Color {
  const inverseForContrast = specifiedTextColorIsDark === backgroundIsDark;
  const { r: tr, g: tg, b: tb, opacity: to } = stringToRGB(textColor);
-  if (!textContrast) {


You know, something about all the conditionals in this fn, are driving me nuts.
It's probably purely academic, but the fact that it checks for truthy against
inverseForContrast three times keeps bothering me.

Again, not worth it, but it flows terribly in my eyes, just a personal style nit though.
Not worth upsetting anyone. I did the same code elsewhere in Kibana, and it still bothers me.
I just didnt feel like writing my own cond fn lol.

Cond
A fn takes a list of predicate fns and then executes different fns, based on the predicates passed in. I'm not even sure how best to impl that. Interesting thought exercise though.

Kinda like: https://ramdajs.com/docs/#cond

I'm just venting emotionally though hahahahaaa

What I did is just as "unattractive": https://github.com/elastic/kibana/blob/master/src/dev/code_coverage/ingest_coverage/either.js#L49

monfera · 2021-09-02T11:28:40Z

jenkins test this

monfera · 2021-09-02T18:00:20Z

packages/charts/src/common/color_calcs.ts

@@ -142,6 +142,10 @@ export function colorIsDark(color: Color): boolean {
  return luminance < 0.2;
 }

+const reducer = (acc: any, curr: any, idx: any) => (idx !== 0 ? `${acc}, ${curr}` : `${curr}`);
+
+const invert = (r: number, g: number, b: number) => [r, g, b].map((x) => 255 - x).reduce(reducer, '');


[].join(', ') removes the need for the [].reduce and the overly generically named reducer, though even with that, the mild repetition

const invert = (r: number, g: number, b: number) => `${255 - r}, ${255 - g}, ${255 - b}`

would likely lead to fewer cycles, fewer bytes allocated and a bit shorter code, plus the code reader sees what's going on on a 1st blink. I wouldn't name it invert though as it returns a partial CSS color string, besides the inversion.

Combined with Marko's pitch for using rgba even if rgb would suffice, it could be

const invertedColor = (r: number, g: number, b: number, a: number) => `rgba(${255 - r}, ${255 - g}, ${255 - b}, a)`

Ahhh, ok. That makes a lot of sense. I'll update.

Does that mean rgb and rgbA are nearly interchangeable?

Ahh, I see, it's about opacity. I get it now.

wayneseymour · 2021-09-06T08:44:41Z

jenkins test this

Refactor to use truthy, remove a little duplication using native bind method. Signed-off-by: Tre' Seymour <[email protected]>

with ternary expression. Less code is the best code :)

parameter list. Also, remove partial application.

concise.

Use Marco's inversion fn instead, also lifted. Make it a little DRY-er.

I'm not super happy with this commit. I've considered binding the makeHighContrastColor fn, and partially applying it, but the amount of code that takes seems not worth it. This way, at least it's only evaluated if textContrast is a number. We could also lift this out of this fn, and instead of using hoisting, it'd be truly first-class, but then the parameter list would be huge. Lots of pros and cons, but the vrt is happy!

monfera · 2021-09-09T09:29:46Z

@wayneseymour the resolution for the axis_utils.ts conflict is this:

  return axisSpec.showDuplicatedTicks ? allTicks : getUniqueValues(allTicks, 'axisTickLabel', true);

I did this conversion to ternary when we looked at axis code together, but then I also introduced axisTickLabel so you can just accept the current one on merge

wayneseymour · 2021-09-09T09:35:21Z

Running the vrt now on my latest changes, if it passes, I'll push

…tor/triple-equals-true

packages/charts/src/chart_types/xy_chart/utils/axis_utils.ts

wayneseymour · 2021-09-09T11:18:32Z

packages/charts/src/common/color_calcs.ts

-      : makeHighContrastColor(textColor, backgroundColor, textContrast);
+        ? makeHighContrastColor(`rgb(${inverted})`, backgroundColor, ratio || undefined)
+        : makeHighContrastColor(`rgba(${inverted}, ${to})`, backgroundColor, ratio || undefined)
+      : makeHighContrastColor(textColor, backgroundColor, ratio || undefined);


Now that I think about it, this could be simply , ratio...should be undefined if it's not passed.

@monfera thoughts?

I can explain my way why we should refactor this function in a different way:

calling makeHighContrastColor with an rgba or rgb color is exactly the same. The internal code take care of the values.

we can always write an rgba color starting from an rgb color adding opacity 1

the ratio or is defined and a number if not it should be left undefined living the makeHighContrastColor use the default value

export function getTextColorIfTextInvertible( textColorIsDark: boolean, backgroundIsDark: boolean, textColor: Color, textContrast: TextContrast, backgroundColor: Color, ): Color { const finalTextColor = textColorIsDark === backgroundIsDark ? invertColor(stringToRGB(textColor)) : textColor; const contrastRatio = typeof textContrast === 'number' ? textContrast : undefined; return textContrast === false ? finalTextColor : makeHighContrastColor(finalTextColor, backgroundColor, contrastRatio); } function invertColor({ r, g, b, opacity }: RgbObject): Color { return `rgba(${255 - r}, ${255 - g}, ${255 - b}, opacity: ${opacity})`; }

I have some doubts on the real need of this function anyway, but if we extract it out of the context this is my solution

wayneseymour · 2021-09-09T12:29:36Z

jenkins test this

wayneseymour · 2021-09-09T15:30:31Z

jenkins test this

wayneseymour force-pushed the refactor/triple-equals-true branch from f40d049 to a095403 Compare September 1, 2021 15:00

monfera changed the title ~~refactor[code improvements]: Drop triple equals true on booleans~~ refactor(xy): drop triple equals true on booleans Sep 1, 2021

monfera reviewed Sep 1, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/utils/axis_utils.ts Outdated Show resolved Hide resolved

monfera reviewed Sep 1, 2021

View reviewed changes

wayneseymour commented Sep 2, 2021

View reviewed changes

monfera reviewed Sep 2, 2021

View reviewed changes

wayneseymour added 9 commits September 8, 2021 11:22

refactor[code improvements]: Drop triple equals true on booleans

3807895

Refactor to use truthy, remove a little duplication using native bind method. Signed-off-by: Tre' Seymour <[email protected]>

Replace conditional return and return statement,

efb98ab

with ternary expression. Less code is the best code :)

Drop 't' prefix to color fn's

825e9d4

parameter list. Also, remove partial application.

Try to reduce the names into something

bdfaa3f

concise.

Lift out a fn.

5f5b06a

Use Marco's inversion fn instead, also lifted. Make it a little DRY-er.

Looks like the vrt harness is happy.

fb135a2

Drop curlies.

965cebb

Simplify to map and join, per CR.

10debfd

wayneseymour added 2 commits September 9, 2021 11:26

Merge branch 'master' of github.com:elastic/elastic-charts into refac…

75097b4

…tor/triple-equals-true

Drop contrasting to one fn.

d59bdd2

wayneseymour force-pushed the refactor/triple-equals-true branch from 4219fbd to d59bdd2 Compare September 9, 2021 11:13

wayneseymour commented Sep 9, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/utils/axis_utils.ts Outdated Show resolved Hide resolved

wayneseymour commented Sep 9, 2021

View reviewed changes

Drop unneeded carriage return.

3182b2c

wayneseymour marked this pull request as ready for review September 9, 2021 15:25

wayneseymour requested a review from monfera September 9, 2021 15:26

wayneseymour requested a review from markov00 September 9, 2021 15:46

Merge branch 'master' into refactor/triple-equals-true

439dd8d

wayneseymour closed this Sep 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(xy): drop triple equals true on booleans #1350

refactor(xy): drop triple equals true on booleans #1350

wayneseymour commented Sep 1, 2021 •

edited

Loading

wayneseymour commented Sep 1, 2021

monfera Sep 1, 2021

wayneseymour Sep 2, 2021

markov00 Sep 2, 2021

wayneseymour Sep 2, 2021

wayneseymour Sep 2, 2021

wayneseymour Sep 2, 2021

monfera commented Sep 1, 2021

wayneseymour Sep 2, 2021

wayneseymour Sep 2, 2021

monfera commented Sep 2, 2021

monfera Sep 2, 2021 •

edited

Loading

wayneseymour Sep 6, 2021

wayneseymour Sep 6, 2021

wayneseymour Sep 6, 2021

wayneseymour commented Sep 6, 2021

monfera commented Sep 9, 2021

wayneseymour commented Sep 9, 2021

wayneseymour Sep 9, 2021

wayneseymour Sep 9, 2021

markov00 Sep 9, 2021

wayneseymour commented Sep 9, 2021

wayneseymour commented Sep 9, 2021

refactor(xy): drop triple equals true on booleans #1350

refactor(xy): drop triple equals true on booleans #1350

Conversation

wayneseymour commented Sep 1, 2021 • edited Loading

Summary

Details

Issues

wayneseymour commented Sep 1, 2021

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

monfera commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera commented Sep 2, 2021

monfera Sep 2, 2021 • 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

wayneseymour commented Sep 6, 2021

monfera commented Sep 9, 2021

wayneseymour commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wayneseymour commented Sep 9, 2021

wayneseymour commented Sep 9, 2021

wayneseymour commented Sep 1, 2021 •

edited

Loading

monfera Sep 2, 2021 •

edited

Loading