refactor(cartesian): timeslip preparation #1286

monfera · 2021-08-05T22:03:21Z

Summary

Non-functional code cleanup, code removal and type fixes. Also removes the large pixel dimensions with texture use.

BREAKING CHANGE

TextStyle.fontStyle is no longer a string, it's the more specific FontStyle
For symmetry, fontStyle in word cloud is also switching from string to FontStyle
Certain text configurations included both fill and textColor for the text color; fill got removed, because textColor is part of the public Font type, and because textColor has clearer meaning than fill. Yet, some of the code used the fill property and/or made the fill property also mandatory. So, userland code needs to remove some fill property, and might need to ensure that the correct value is going into textColor
getRadians got unpublished

Details

Reviewers,

commit 848e188d866be40c79bc545dbf4cac81c8f347ce contains the breaking change of switching to FontStyle
commit 80c743fa879003a986a2e78d0e297229e712edf2 contains the breaking change of phasing out fill
commit b544c0c9364b1e14c3e4816a6de120f92ba35e8f unpublishes getRadian (no apparent change to API MD)

Issues

Checklist

The proper chart type label was added (e.g. :xy, :partition) if the PR involves a specific chart type
The proper feature label was added (e.g. :interactions, :axis) if the PR involves a specific chart feature
Any consumer-facing exports were added to packages/charts/src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility

monfera · 2021-08-06T00:02:53Z

jenkins test this

monfera · 2021-08-06T01:42:51Z

jenkins test this

monfera · 2021-08-06T09:58:31Z

packages/charts/src/chart_types/xy_chart/renderer/canvas/panels/panel_title.ts

-    fontStyle: fontStyle ? (fontStyle as FontStyle) : 'normal',
-    fontWeight: 'normal',
+    fontStyle: fontStyle ?? 'normal',
+    fontWeight: 'bold',


We could introduce an API option for font weight instead, then this bold can go back into the theme

monfera · 2021-08-06T10:05:30Z

packages/charts/src/utils/themes/theme.test.ts

        fontSize: 50,
        fontFamily: 'custom-font-family',
-        fontStyle: 'custom-font-style',
+        fontStyle: 'italic',


If there's a real need for 'custom-font-style' we could introduce a fontShorthand or fontOverride property, the presence of which would bypass the current CSS shorthand string concatenation logic

monfera · 2021-08-06T11:24:53Z

packages/charts/src/utils/common.tsx

- */
-export const getRadians = (angle: number) => (angle * Math.PI) / 180;
+/** @internal */
+export const degToRad = (angle: Degrees): Radian => (angle / 180) * Math.PI;


Unpublishing it as we don't currently expose pure utility functions in our API, and it wouldn't be a good fit for our library because the related areas take in angles in degrees, so the user needs not convert to radians

monfera · 2021-08-06T16:39:02Z

packages/charts/src/renderers/canvas/index.ts

-    ctx.translate(x, y);
-    ctx.rotate(getRadians(rotation));
-    ctx.translate(-x, -y);
-    fn(ctx);


Removed this function, b/c a single place used it, and it immediately reverted the last (negative) translate here

packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/rect.ts

monfera · 2021-08-07T00:42:16Z

packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/rect.ts

-    const height = rect.height - borderOffset * 2;
-
+  const borderWidth = !disableBorderOffset && stroke.width >= MIN_STROKE_WIDTH ? stroke.width : 0;
+  if (stroke.width >= MIN_STROKE_WIDTH) {


A bunch of 0.001's got replaced with MIN_STROKE_WIDTH

… averted

rshen91 · 2021-08-09T15:50:30Z

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

-    x: 0,
-    y: 0,
-    rotate: 0,
+    x: chartRotation === 90 || chartRotation === 180 ? width : 0,


Nice clean up!

O(n) 🎉

rshen91

Thanks for doing this clean up. Tested locally and changes look great!

nickofthyme

LGTM, the vr tests do not like your changes 🤣 . Idk what that annotation story always triggers a small diff for all.

packages/charts/api/charts.api.md

packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/rect.ts

nickofthyme · 2021-08-09T16:51:58Z

packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/text.ts

@@ -26,30 +27,28 @@ export function renderText(
  origin: Point,
  text: string,
  font: TextFont,
-  degree: number = 0,
-  translation?: Partial<Point>,
+  angle: Degrees = 0,


nickofthyme · 2021-08-09T16:53:20Z

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

-    x: 0,
-    y: 0,
-    rotate: 0,
+    x: chartRotation === 90 || chartRotation === 180 ? width : 0,


O(n) 🎉

nickofthyme · 2021-08-09T16:54:07Z

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

@@ -76,7 +76,16 @@ function createPattern(
    if (fill) pCtx.fill(path);
  }

-  return ctx.createPattern(patternCanvas, 'repeat') ?? undefined;
+  const pattern = ctx.createPattern(patternCanvas, 'repeat')!; // HTMLCanvasElement always yields a CanvasPattern anyway


So this will never ever throw?

It won't throw, as the spec says ctx.createPattern invoked with HTMLCanvasElement (which patternCanvas is guaranteed to be) has no null case.

Details:

It'd throw downstream of this function if pattern ended up being null (there are other throw conditions but TS guards against most(*) of those already; example: the "repeat" literal is OK here).

The spec for createPattern says:

The only potential for getting a null is point 2:

If usability is bad, then return null

It's the preceding point that references usability:

Let usability be the result of checking the usability of image

The "switch" at the link has no "bad" usability case for HTMLCanvasElement, so there can't be a null, so the ! is safe and pattern will never be null. Typescript (apparently) is just not granular enough here to recognize that certain argument types can't lead to certain result types.

(*) If my 2nd reading prompted by your question is correct, then a zero dimension canvas would lead to a DOM exception. TS doesn't complain about this possibility, nor does current master wrap it in throw/catch so this aspect is not made worse by this PR. Still, I'll check if we defend against a zero-dimension canvas upstream, eg. by not creating such a canvas, or by applying a minimum 1px by 1px dimension. The best combo is,avoiding a throw condition so that a throw/catch wrapper is not needed

zero dimension canvas would lead to a DOM exception

Good to know!

Added this and a bunch of other things here into #1303

# Conflicts: # packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/rect.ts

monfera · 2021-08-16T16:13:43Z

jenkins test this

monfera · 2021-08-16T17:03:45Z

jenkins test this

nickofthyme

Latest changes LGTM.

# [34.0.0](v33.2.4...v34.0.0) (2021-08-16) ### Code Refactoring * **cartesian:** cartesian rendering iteration ([#1286](#1286)) ([b2ae4f7](b2ae4f7)), closes [#1202](#1202) ### BREAKING CHANGES * **cartesian:** - `TextStyle.fontStyle` is no longer a `string`, it's the more specific `FontStyle` - For symmetry, `fontStyle` in word cloud is also switching from `string` to `FontStyle` - Certain text configurations included both `fill` and `textColor` for the text color; `fill` got removed, because `textColor` is part of the public `Font` type, and because `textColor` has clearer meaning than `fill`. Yet, some of the code used the `fill` property and/or made the `fill` property also mandatory. So, userland code needs to remove some `fill` property, and might need to ensure that the correct value is going into `textColor` - `getRadians` got unpublished - No attempt to draw a rect border if there's not enough width/height for at least the specified border width (ie. width/height being at least twice the border width)

nickofthyme · 2021-08-16T23:12:15Z

🎉 This PR is included in version 34.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

monfera added :xy Bar/Line/Area chart related code quality labels Aug 5, 2021

monfera marked this pull request as draft August 5, 2021 22:03

monfera force-pushed the timeslip-prep branch 2 times, most recently from af9efea to 1d5aea6 Compare August 5, 2021 22:52

monfera added the :axis Axis related issue label Aug 5, 2021

monfera force-pushed the timeslip-prep branch 3 times, most recently from d3e3e2b to 848e188 Compare August 5, 2021 23:37

monfera added :heatmap Heatmap/Swimlane chart related issue :wordcloud Wordcloud related issues labels Aug 6, 2021

monfera commented Aug 6, 2021

View reviewed changes

monfera force-pushed the timeslip-prep branch 2 times, most recently from 210f2d8 to cc63a3b Compare August 6, 2021 16:33

monfera commented Aug 6, 2021

View reviewed changes

monfera requested a review from nickofthyme August 6, 2021 17:40

monfera commented Aug 7, 2021

View reviewed changes

packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/rect.ts Show resolved Hide resolved

monfera commented Aug 7, 2021

View reviewed changes

monfera requested review from markov00 and rshen91 August 7, 2021 00:44

monfera added 6 commits August 7, 2021 02:47

chore: reverted earlier pr change

52f16ad

refactor: simpler rendering code

74fdba5

refactor: simpler rendering code 2

9ba5b74

refactor: renderText is already wrapped in withContext

8981cbc

refactor: withRotatedOrigin already wraps via withContext

6821d7a

chore: no need for condition

f0d5781

monfera force-pushed the timeslip-prep branch from 2ab0df5 to 82619f6 Compare August 7, 2021 00:47

monfera added 5 commits August 7, 2021 10:51

refactor: dry for transform

aced8f6

refactor: single use non documentational variable inlined and returns…

a3acdec

… averted

refactor: flipped if for common case

11cee41

refactor: single element curlies removed

39425e0

fix: restored lineCap

9351126

rshen91 reviewed Aug 9, 2021

View reviewed changes

rshen91 self-requested a review August 9, 2021 16:01

rshen91 approved these changes Aug 9, 2021

View reviewed changes

nickofthyme reviewed Aug 9, 2021

View reviewed changes

monfera marked this pull request as ready for review August 12, 2021 22:03

monfera added 3 commits August 16, 2021 17:16

Merge remote-tracking branch 'origin/master' into timeslip-prep

b738caa

# Conflicts: # packages/charts/src/chart_types/xy_chart/renderer/canvas/primitives/rect.ts

chore: merge of rect change from elastic#1281 into this pr

d1d1bdd

chore: also check the width

6837ebb

monfera mentioned this pull request Aug 16, 2021

Code improvements meta issue #1303

Open

19 tasks

test: also check the width vrt image update

b8608c3

nickofthyme approved these changes Aug 16, 2021

View reviewed changes

nickofthyme and others added 2 commits August 16, 2021 12:53

Merge branch 'master' into timeslip-prep

9eb6665

Merge branch 'master' into timeslip-prep

8437716

monfera enabled auto-merge (squash) August 16, 2021 18:52

monfera merged commit b2ae4f7 into elastic:master Aug 16, 2021

nickofthyme added the released Issue released publicly label Aug 16, 2021

This was referenced Aug 17, 2021

Improve texture coloring on bar/area #1202

Open

chore(xy): avoid large dimensions in ctx.clearRect #1307

Merged

nickofthyme mentioned this pull request Aug 30, 2021

[ML] fix heatmap label colors elastic/kibana#110515

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cartesian): timeslip preparation #1286

refactor(cartesian): timeslip preparation #1286

monfera commented Aug 5, 2021 •

edited

monfera commented Aug 6, 2021

monfera commented Aug 6, 2021

monfera Aug 6, 2021

markov00 Aug 16, 2021

monfera Aug 6, 2021 •

edited

monfera Aug 6, 2021

monfera Aug 6, 2021

monfera Aug 7, 2021

rshen91 Aug 9, 2021

nickofthyme Aug 9, 2021

rshen91 left a comment

nickofthyme left a comment

nickofthyme Aug 9, 2021

nickofthyme Aug 9, 2021

nickofthyme Aug 9, 2021

monfera Aug 12, 2021 •

edited

nickofthyme Aug 16, 2021

monfera Aug 16, 2021

monfera commented Aug 16, 2021

monfera commented Aug 16, 2021

nickofthyme left a comment

nickofthyme commented Aug 16, 2021

refactor(cartesian): timeslip preparation #1286

refactor(cartesian): timeslip preparation #1286

Conversation

monfera commented Aug 5, 2021 • edited

Summary

BREAKING CHANGE

Details

Issues

Checklist

monfera commented Aug 6, 2021

monfera commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera Aug 6, 2021 • edited

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

rshen91 left a comment

Choose a reason for hiding this comment

nickofthyme 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

monfera Aug 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfera commented Aug 16, 2021

monfera commented Aug 16, 2021

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme commented Aug 16, 2021

monfera commented Aug 5, 2021 •

edited

monfera Aug 6, 2021 •

edited

monfera Aug 12, 2021 •

edited