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.

Sign up for GitHub

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

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

Closed
wants to merge 13 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3807895
refactor[code improvements]: Drop triple equals true on booleans
wayneseymour Sep 1, 2021
efb98ab
Replace conditional return and return statement,
wayneseymour Sep 2, 2021
825e9d4
Drop 't' prefix to color fn's
wayneseymour Sep 2, 2021
bdfaa3f
Try to reduce the names into something
wayneseymour Sep 2, 2021
5f5b06a
Lift out a fn.
wayneseymour Sep 2, 2021
fb135a2
Looks like the vrt harness is happy.
wayneseymour Sep 2, 2021
965cebb
Drop curlies.
wayneseymour Sep 2, 2021
f866fea
Refactor out duplication to 2 fns...
wayneseymour Sep 2, 2021
10debfd
Simplify to map and join, per CR.
wayneseymour Sep 8, 2021
75097b4
Merge branch 'master' of github.com:elastic/elastic-charts into refac…
wayneseymour Sep 9, 2021
d59bdd2
Drop contrasting to one fn.
wayneseymour Sep 9, 2021
3182b2c
Drop unneeded carriage return.
wayneseymour Sep 9, 2021
439dd8d
Merge branch 'master' into refactor/triple-equals-true
wayneseymour Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/charts/src/chart_types/xy_chart/utils/axis_utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ export function enableDuplicatedTicks(
axisTickLabel: (axisSpec.labelFormat ?? axisSpec.tickFormat ?? fallBackTickFormatter)(tick, tickFormatOptions),
position: (scale.scale(tick) ?? 0) + offset,
}));

wayneseymour marked this conversation as resolved.
Show resolved Hide resolved
return axisSpec.showDuplicatedTicks ? allTicks : getUniqueValues(allTicks, 'axisTickLabel', true);
}

Expand Down
35 changes: 17 additions & 18 deletions packages/charts/src/common/color_calcs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ export function colorIsDark(color: Color): boolean {
return luminance < 0.2;
}

const invert = (r: number, g: number, b: number) => [r, g, b].map((x) => 255 - x).join(',');

/**
* inverse color for text
* @internal
Expand All @@ -159,27 +161,24 @@ export function getTextColorIfTextInvertible(
): Color {
const inverseForContrast = specifiedTextColorIsDark === backgroundIsDark;
const { r: tr, g: tg, b: tb, opacity: to } = stringToRGB(textColor);
if (!textContrast) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return inverseForContrast
? to === undefined
? `rgb(${255 - tr}, ${255 - tg}, ${255 - tb})`
: `rgba(${255 - tr}, ${255 - tg}, ${255 - tb}, ${to})`
: textColor;
}
if (textContrast === true) {
return inverseForContrast
? to === undefined
? makeHighContrastColor(`rgb(${255 - tr}, ${255 - tg}, ${255 - tb})`, backgroundColor)
: makeHighContrastColor(`rgba(${255 - tr}, ${255 - tg}, ${255 - tb}, ${to})`, backgroundColor)
: makeHighContrastColor(textColor, backgroundColor);
} else if (typeof textContrast === 'number') {
const inverted = invert(tr, tg, tb);

if (!textContrast)
return inverseForContrast ? (to === undefined ? `rgb(${inverted})` : `rgba(${inverted}, ${to})`) : textColor;

if (textContrast && typeof textContrast !== 'number') return contrast();

if (typeof textContrast === 'number') return contrast(textContrast);

return 'black'; // this should never happen; added it as previously function return type included undefined; todo

function contrast(ratio?: any) {
return inverseForContrast
? to === undefined
? makeHighContrastColor(`rgb(${255 - tr}, ${255 - tg}, ${255 - tb})`, backgroundColor, textContrast)
: makeHighContrastColor(`rgba(${255 - tr}, ${255 - tg}, ${255 - tb}, ${to})`, backgroundColor, textContrast)
: makeHighContrastColor(textColor, backgroundColor, textContrast);
? makeHighContrastColor(`rgb(${inverted})`, backgroundColor, ratio || undefined)
: makeHighContrastColor(`rgba(${inverted}, ${to})`, backgroundColor, ratio || undefined)
: makeHighContrastColor(textColor, backgroundColor, ratio || undefined);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@monfera thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. calling makeHighContrastColor with an rgba or rgb color is exactly the same. The internal code take care of the values.
  2. we can always write an rgba color starting from an rgb color adding opacity 1
  3. 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

}
return 'black'; // this should never happen; added it as previously function return type included undefined; todo
}

/**
Expand Down