refactor: scales by monfera · Pull Request #1410 · elastic/elastic-charts

monfera · 2021-09-25T11:58:32Z

Summary

Refactoring toward making scales more independent of a preconceived tick count, so the future version of getAvailableTicks can try various tick counts, and possibly even extend the domain. This is needed by both the hierarchical time axis and the solution for avoiding duplicate or misplaced ticks. Miscellaneous additional simplifications

Details

Scale.scale() and Scale.pureScale() always return a number instead of the union type (emittingNaN instead of null and downstream checks as before but with Number.isNaN, Number.isFinite, scale(val) || 0 etc. so no behavioral change is expected)
removed the undocumented sortSeriesBy which wasn't exercised by unit tests, VRT or Kibana, yet its former presence required multiple @ts-ignores which are now also removed
solved four type assertions
replaced all the isNaNs with clearer alternatives (it coerces, eg. isNaN(null) and isNaN("42") yield false)
BREAKING: renamed showOverlappingTicks to ticksForCulledLabels
BREAKING: LogScaleOptions.logBase is now a number instead of the object enum LogBase; duplicate enum->number mappers removed too. Shorter, and allows other bases (octal, hex, 1024 etc.)
BREAKING potentially: (not through an API change though): discontinued scaleOrThrow and all other ...orThrow functions, now just handling certain NaN/non-finiteness cases, except for geom string generation. These used to silently swallow errors in past (hypothetical? IRL experienced?) cases where some D3 scale were to return a non-finite value, or even, throw an error, so geoms silently don't show up. The new behavior is that if the D3 scale were to throw, there's an explicit, clear error, and if there's a NaN or similar, then the geom path will have it, and an explicit console log line will be written. There are pros&cons so this may be an item up for debate, as the nature and type of our defensiveness, and swallow vs. expose errors are concerned
reworked some functions which were shallowly but verbosely wrapped in selectors
reworked ScaleContinuous, for example, the constuctor is split to tick count independent part vs tick count dependent part
combined getAvailableTicks and getVisibleTicks into a new getVisibleTicks (for this reason, unit tests that exercised these separately are currently commented out)
renamed some functions
removed syllogisms (where one of the ternary branches is a simple true or false, the ternary is superfluous)
avoid object creation, code noise and line inflation via changing the signature of mergePartial
numerous other code simplifications (esp. in axis_utils), type updates (fewer optional parameters, fewer union types etc.) and minor fixes (eg. non-integer desired tick count)

I also had to backtrack on (stash) more ambitious changes for now, such as improving the scale typing, I timeboxed it and didn't reach the goal in time

Issues

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
The proper feature labels have been added (e.g. :interactions, :axis)
The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
All related issues have been linked (i.e. closes #123, fixes #123)
New public API exports have been added to packages/charts/src/index.ts
Unit tests have been added or updated to match the most common scenarios
The proper documentation and/or storybook story has been added or updated
The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)

…nd range

nickofthyme

Ok I've gone through all the code changes with the exceptions of the scale_continuous.ts. I need to dig a little deeper into that file and start playing around with storybook.

packages/charts/src/chart_types/xy_chart/state/selectors/get_axis_styles.ts

packages/charts/src/chart_types/xy_chart/state/selectors/on_element_out_caller.ts

packages/charts/src/chart_types/xy_chart/state/selectors/on_element_over_caller.ts

packages/charts/src/chart_types/xy_chart/tooltip/tooltip.ts

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

packages/charts/src/scales/scale_continuous.ts

markov00

LGTM, really a lot of changes, but I see the benefits on returning always a number from the scales and all the clean up makes a lot of sense!
Thanks Robert for gardening our code so well

packages/charts/src/chart_types/heatmap/state/selectors/get_x_axis_right_overflow.ts

packages/charts/src/chart_types/xy_chart/state/selectors/on_pointer_move_caller.ts

markov00 · 2021-09-30T14:25:27Z

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

  const filledDataSeries = fillSeries(dataSeries, xValues, xDomain.type);

-  const seriesSortFn = getRenderingCompareFn(sortSeriesBy, (a: SeriesIdentifier, b: SeriesIdentifier) => {
+  const seriesSortFn = getRenderingCompareFn(void 0, (a: SeriesIdentifier, b: SeriesIdentifier) => {


Just remember to grep all void 0

or enable no-void eslint rule

As there was no current demand for retaining sortSeriesBy, its removal got completed: the void 0s are removed altogether f2f27e9

Added ec90371 which turns voids into warnings. It's legit JS and there's no fundamental reason for avoiding it, other than maybe uniform greppability, which isn't very valuable as undefined values can legitimately generated in all kinds of ways like optionals, so not sure when we'd grep for undefined. So, on the fence about discouraging perfectly good JS

The idea to me is to prevent uncommon javascript syntax, so when 3rd party contributions come in they are not confused. I bet you 1 in 5 javascript engineers know what void 0 is. If statements are also perfectly fine JS but yet this PR removes dozens 😉, just saying....

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

packages/charts/src/mocks/series/series.ts

packages/charts/src/scales/scale_continuous.ts

nickofthyme

Still waiting on some file changes, lmk when you want me to take a look.

This reverts commit 252b94d

This reverts commit 685892c

…ay anyway

Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

monfera · 2021-10-05T12:00:11Z

Pls let's preformat our suggestions in our IDE, otherwise our hypersensitive CI will yield a bunch of big red crosses due to a couple of spaces 🤣 Also, we get the "red card" for incomplete test runs, which is awkward, b/c it disincentivizes the otherwise normal process of applying suggestions one after the other, such that there's no time for CO to complete

monfera · 2021-10-05T12:05:13Z

I ended up reverting the "chore: untangle continuous scales file" commit, because it has served its purpose (allowing side by side comparison) and I'd like to avoid merge conflicts in that code. Also, the new order is I think more in line with what Cartesian code seems to follow, which is, put the main thing (class or function) atop, and place most utilities and nuisance bits (looking at you, loong type description) below

nickofthyme

New changes LGTM.

monfera · 2021-10-05T14:30:40Z

Thanks, just saw your reply, let's figure out something over time, as for every verbified function we also have maybe significant or even several non-verbed ones

…

On Tue, 5 Oct 2021 at 15:59, Nick Partridge ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In packages/charts/src/chart_types/xy_chart/utils/axis_utils.ts <#1410 (comment)> : > +/** @internal */ +export function tickLabelPosition( { tickLine, tickLabel }: AxisStyle, This is a tough one because I don't feel this is good JAVASCRIPT best practices. If it comes up again every 3 weeks it will start to get distracting. Until we switch to some other language I don't think is a legitimate concern. Even if we somehow all agree to this we will have to account for dozens of naming collisions (i.e. const tickLabelPosition = tickLabelPosition()) and implement it throughout the library consistently. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1410 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAL2BZCQJYVKNJ67AHFR55TUFMAC5ANCNFSM5EXRGELQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

# [38.0.0](v37.0.0...v38.0.0) (2021-10-15) ### Bug Fixes * **deps:** update dependency @elastic/eui to v39 ([#1422](#1422)) ([2ee97aa](2ee97aa)) * **goal:** reduce whitespace for circular charts ([#1413](#1413)) ([6517523](6517523)) * **interactions:** change allowBrushingLastHistogramBin to true ([#1396](#1396)) ([9fa9783](9fa9783)) * **xy:** remove wrongly represented null/missing values in tooltip ([#1415](#1415)) ([e5963a3](e5963a3)), closes [#1414](#1414) ### Code Refactoring * scales ([#1410](#1410)) ([a53a2ba](a53a2ba)) ### Features * **scales:** add `LinearBinary` scale type ([#1389](#1389)) ([9f2e427](9f2e427)) * **xy:** adaptive tick raster ([#1420](#1420)) ([200577b](200577b)) * **xy:** apply the data value formatter to data values over bars ([#1419](#1419)) ([e673fc7](e673fc7)) ### BREAKING CHANGES * **interactions:** allowBrushingLastHistogramBucket renamed to allowBrushingLastHistogramBin on the Settings component defaults true and is only applied for histogram type charts * LogScaleOptions.logBase` is now a `number` instead of the object enum `LogBase`. Some edge case data or configuration _might_, with a deemed low likelihood, lead to a situation where the earlier version would have silently not rendered a bar, line or point, while the new code doesn't `catch`, therefore throw an exception (see the last item). General risk of regressions due to the quantity of code changes (altogether 3.5k)

nickofthyme · 2021-10-15T14:01:22Z

🎉 This PR is included in version 38.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

monfera added 17 commits September 25, 2021 00:44

refactor: remove some default values like ScaleData required domain a…

7d0241e

…nd range

refactor: getSpecsFromStore simplification

14e36af

refactor: isClicking simplification

ab728f6

chore: remove redundant bbox type

848504f

chore: mandatory title

685892c

refactor: name clarification for meaning

4e378f2

test: textMeasure test update for mandatory arguments

53a8df9

refactor: simpler getAxesDimensions

da234bc

refactor: guarantee fresh object and no benefit of extraction

995f5a9

chore: remove an assertion

031cf01

chore: remove another assertion

3de1f9f

chore: avoid a possibly infinite loop

c537e7c

chore: avoid a possibly infinite loop 2

869f7ba

chore: a bit more safety for log scales

710218c

chore: remove disused property ht MarcoV

fcaba5a

refactor: inline single use variable

5715651

refactor: single exit point

eae74ad

monfera marked this pull request as draft September 25, 2021 11:58

monfera changed the title ~~tmp~~ refactor: scales Sep 25, 2021

monfera added 11 commits September 25, 2021 14:31

refactor: merging tick functions 1

3778d62

refactor: merging tick functions 2

941c525

refactor: merging tick functions 3 - test deactivation

8e5f4a5

refactor: flatter function

70a1e88

refactor: flatter function 2

6989c93

refactor: flatter function 3

5b5da23

chore: remove some curly and space noise

9108fb7

fix: remove an assertion

3da18ce

refactor: getPixelPaddedDomain for stp

5bdda43

refactor: non-optional param

bbd7c8c

refactor: remove noisy abstraction 1

7a44a16

chore: untangle continuous scales file

252b94d

nickofthyme reviewed Sep 30, 2021

View reviewed changes

markov00 reviewed Oct 1, 2021

View reviewed changes

markov00 approved these changes Oct 4, 2021

View reviewed changes

nickofthyme requested changes Oct 4, 2021

View reviewed changes

monfera added 6 commits October 5, 2021 11:52

Revert "chore: untangle continuous scales file"

9a96a00

This reverts commit 252b94d

chore: removed the former vestigial void 0 arguments

f2f27e9

Revert "chore: mandatory title"

4ba73bb

This reverts commit 685892c

chore: void 0 switched to warning level

d4c923e

chore: reverted the renaming of showOverlappingTicks as it will go aw…

d7dc0d8

…ay anyway

Merge remote-tracking branch 'origin/master' into time-axis-07

ed6a3c0

monfera force-pushed the time-axis-07 branch from 9b9dea6 to ed6a3c0 Compare October 5, 2021 11:02

monfera and others added 6 commits October 5, 2021 13:10

chore: used a verb

6fe5921

chore: reify isOutElement as booean

c994a15

Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>

chore: remove an unnecessary line, part 1

cf24d2d

Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>

chore: remove an unnecessary line, part 2

73855b6

Co-authored-by: Nick Partridge <nick.ryan.partridge@gmail.com>

chore: line breaking fix for suggested commit

802334d

chore: further simplified hasPointerEventChanged

50269be

Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

monfera requested a review from nickofthyme October 5, 2021 12:00

nickofthyme approved these changes Oct 5, 2021

View reviewed changes

monfera merged commit a53a2ba into elastic:master Oct 5, 2021

nickofthyme mentioned this pull request Oct 5, 2021

fix(theme): allow theme overrides to be accepted #1392

Closed

nickofthyme mentioned this pull request Oct 14, 2021

Theme overrides are not applied with missing optional base values #1284

Closed

nickofthyme added the released Issue released publicly label Oct 15, 2021

markov00 mentioned this pull request Jan 3, 2023

integersOnly is no longer working #1920

Closed

2 tasks

Conversation

monfera commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Issues

Checklist

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

markov00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

markov00 Sep 30, 2021

Choose a reason for hiding this comment

Uh oh!

nickofthyme Oct 1, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

monfera Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

monfera commented Oct 5, 2021

Uh oh!

monfera commented Oct 5, 2021

Uh oh!

nickofthyme left a comment

Choose a reason for hiding this comment

Uh oh!

monfera commented Oct 5, 2021 via email

Uh oh!

nickofthyme commented Oct 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

monfera commented Sep 25, 2021 •

edited

Loading

monfera Oct 5, 2021 •

edited

Loading