-
Notifications
You must be signed in to change notification settings - Fork 121
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.
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
fix: chart state and series functions cleanup #989
Changes from 25 commits
8a910c1
e62a4b8
ef613c9
644c1d0
d930263
ff1d169
5f4f7a1
092a2b1
b18594a
2e1c958
02858be
a050312
cd11ecc
07c7509
e157b16
1cec05e
181ba19
0c72f25
878b2d2
e06d62f
11091b0
9058c10
95fb4e1
3e2d1c7
76dfdf6
3545cd8
17e1356
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,20 +19,20 @@ | |
|
||
import { SeriesIdentifier, SeriesKey } from '../../../common/series_id'; | ||
import { ScaleType } from '../../../scales/constants'; | ||
import { GroupBySpec, BinAgg, Direction, XScaleType } from '../../../specs'; | ||
import { BinAgg, Direction, GroupBySpec, XScaleType } from '../../../specs'; | ||
import { OrderBy } from '../../../specs/settings'; | ||
import { ColorOverrides } from '../../../state/chart_state'; | ||
import { Accessor, AccessorFn, getAccessorValue } from '../../../utils/accessor'; | ||
import { Datum, Color, isNil } from '../../../utils/common'; | ||
import { Color, Datum, isNil } from '../../../utils/common'; | ||
import { GroupId } from '../../../utils/ids'; | ||
import { Logger } from '../../../utils/logger'; | ||
import { ColorConfig } from '../../../utils/themes/theme'; | ||
import { groupSeriesByYGroup, isHistogramEnabled, isStackedSpec } from '../domains/y_domain'; | ||
import { LastValues } from '../state/utils/types'; | ||
import { applyFitFunctionToDataSeries } from './fit_function_utils'; | ||
import { groupBy } from './group_data_series'; | ||
import { BasicSeriesSpec, SeriesTypes, SeriesSpecs, SeriesNameConfigOptions, StackMode } from './specs'; | ||
import { formatStackedDataSeriesValues, datumXSortPredicate } from './stacked_series_utils'; | ||
import { BasicSeriesSpec, SeriesNameConfigOptions, SeriesSpecs, SeriesTypes, StackMode } from './specs'; | ||
import { datumXSortPredicate, formatStackedDataSeriesValues } from './stacked_series_utils'; | ||
nickofthyme marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** @internal */ | ||
export const SERIES_DELIMITER = ' - '; | ||
|
@@ -115,9 +115,6 @@ export function getSeriesIndex(series: SeriesIdentifier[], target: SeriesIdentif | |
|
||
/** | ||
* Returns string form of accessor. Uses index when accessor is a function. | ||
* | ||
* @param accessor | ||
* @param index | ||
* @internal | ||
*/ | ||
export function getAccessorFieldName(accessor: Accessor | AccessorFn, index: number) { | ||
|
@@ -126,7 +123,7 @@ export function getAccessorFieldName(accessor: Accessor | AccessorFn, index: num | |
|
||
/** | ||
* Split a dataset into multiple series depending on the accessors. | ||
* Each series is then associated with a key thats belong to its configuration. | ||
* Each series is then associated with a key that belongs to its configuration. | ||
* This method removes every data with an invalid x: a string or number value is required | ||
* `y` values and `mark` values are casted to number or null. | ||
* @internal | ||
|
@@ -329,12 +326,7 @@ function castToNumber(value: any, nonNumericValues: any[]): number | null { | |
return num; | ||
} | ||
|
||
/** | ||
* Sorts data based on order of xValues | ||
* @param dataSeries | ||
* @param xValues | ||
* @param xScaleType | ||
*/ | ||
/** Sorts data based on order of xValues */ | ||
const getSortedDataSeries = ( | ||
dataSeries: DataSeries[], | ||
xValues: Set<string | number>, | ||
|
@@ -380,14 +372,7 @@ export function getFormattedDataSeries( | |
return [...fittedAndStackedDataSeries, ...nonStackedDataSeries]; | ||
} | ||
|
||
/** | ||
* | ||
* @param seriesSpecs the map for all the series spec | ||
* @param deselectedDataSeries the array of deselected/hidden data series | ||
* @param enableVislibSeriesSort is optional; if not specified in <Settings />, | ||
* @param smallMultiples | ||
* @internal | ||
*/ | ||
/** @internal */ | ||
export function getDataSeriesFromSpecs( | ||
seriesSpecs: BasicSeriesSpec[], | ||
deselectedDataSeries: SeriesIdentifier[] = [], | ||
|
@@ -518,6 +503,8 @@ function getSortedOrdinalXValues( | |
} | ||
} | ||
|
||
const BIG_NUMBER = Number.MAX_SAFE_INTEGER; // the sort comparator must yield finite results, can't use infinities | ||
markov00 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function getSeriesNameFromOptions( | ||
options: SeriesNameConfigOptions, | ||
{ yAccessor, splitAccessors }: XYChartSeriesIdentifier, | ||
|
@@ -530,7 +517,7 @@ function getSeriesNameFromOptions( | |
return ( | ||
options.names | ||
.slice() | ||
.sort(({ sortIndex: a = Infinity }, { sortIndex: b = Infinity }) => a - b) | ||
.sort(({ sortIndex: a = BIG_NUMBER }, { sortIndex: b = BIG_NUMBER }) => a - b) | ||
.map(({ accessor, value, name }) => { | ||
const accessorValue = splitAccessors.get(accessor) ?? null; | ||
if (accessorValue === value) { | ||
|
@@ -557,41 +544,28 @@ export function getSeriesName( | |
isTooltip: boolean, | ||
spec?: BasicSeriesSpec, | ||
): string { | ||
let delimiter = SERIES_DELIMITER; | ||
if (spec && spec.name && typeof spec.name !== 'string') { | ||
let customLabel: string | number | null = null; | ||
if (typeof spec.name === 'function') { | ||
customLabel = spec.name(seriesIdentifier, isTooltip); | ||
} else { | ||
delimiter = spec.name.delimiter ?? delimiter; | ||
customLabel = getSeriesNameFromOptions(spec.name, seriesIdentifier, delimiter); | ||
} | ||
|
||
if (customLabel !== null) { | ||
return customLabel.toString(); | ||
} | ||
const customLabel = | ||
typeof spec?.name === 'function' | ||
? spec.name(seriesIdentifier, isTooltip) | ||
: typeof spec?.name === 'object' // extract booleans once https://github.com/microsoft/TypeScript/issues/12184 is fixed | ||
? getSeriesNameFromOptions(spec.name, seriesIdentifier, spec.name.delimiter ?? SERIES_DELIMITER) | ||
: null; | ||
Comment on lines
+547
to
+552
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems harder to read for me TBH, not sure why the nested ternary lines are not indented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's not nesting b/c it's a linear fallthrough case, ie. no structural nesting applies, more like a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a ternary inside another ternary, that's nested no? As far as the indenting, this is what I'd like to see that delineates the const customLabel =
typeof spec?.name === 'function'
? spec.name(seriesIdentifier, isTooltip)
: typeof spec?.name === 'object' // extract booleans once https://github.com/microsoft/TypeScript/issues/12184 is fixed
? getSeriesNameFromOptions(spec.name, seriesIdentifier, spec.name.delimiter ?? SERIES_DELIMITER)
: null; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This formatting would be OK too, maybe in some formatting PR we can test drive how the rule would shape the ternaries here and elsewhere.
Especially with code I didn't write, I find it way easier to linearly step through, "OK this condition still not true, let's look whether the next applies" compared to needing to simulate ascending and descending into (semantically, not just AST-wise) nested Languages: JS doesn't offer a true fib n | n == 0 = 1
| n == 1 = 1
| n >= 2 = fib (n-1) + fib (n-2) again, linear/rectangular code, easy to read top to bottom, and a single pass reading gives a good picture, no need to ascend and descend into conditional valleys. JS don't have no such pattern matching... Bigger picture: Both code (internal view, our DX) and API (external view, Kibana DX) benefit from a strive toward simplicity. One way for achieving it is to reduce the number of conditional paths, nesting variety and one-off handling. It's always straightforward to add to the code; "on this condition X let's do something different before/after/instead of the current thing", which is how code bases become tangled. So my feel is that the real solution is not to turn into ternary all such bushy Instead, we could be be a bit slower to add features; let's brainstorm if
That's the drawback with broadening types into union types (discriminated or not), and/or optional props, and the error prone falsey and fallthrough cases that require mental juggling and exponentially growing conditional branches like We talked about eventually farming out a humane layer for somewhat hairy rules, defaults and fallbacks, so our core isn't penetrated by API use brevity or other architecture wise superficially manageable reasons; ie. core charting, wrapped around with defaults etc. driven by best practices, folks' expectations, customs etc. We had a chat with @markov00 today about an approach toward layered, priority/fallback driven configuration, which doesn't bake in what the user can and cannot override along some preconceived directions. So either of these options could help avoid expoding conditionals. So ultimately I agree, and work toward not needing this many layers of "pattern matching", ternaries or not. Future PRs from all of us FTW! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks as always for the thoughtful reply. I just don't want to get to a point where we are forming super complex ternaries to remove complex nested conditionals. Replacing some makes sense while others would be hard to understand, but yes reducing the bushyness would be great. As for the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @nickofthyme! Great idea for hunting for disused functionality (esp. that's not foreseen to be used; maybe this I agree the ternary chaining is less than optimal, there's room for improving on it, hopefully by simplification rather than eg. |
||
|
||
if (customLabel !== null) { | ||
return customLabel.toString(); | ||
} | ||
|
||
let name = ''; | ||
const nameKeys = | ||
spec && spec.yAccessors.length > 1 ? seriesIdentifier.seriesKeys : seriesIdentifier.seriesKeys.slice(0, -1); | ||
|
||
// there is one series, the is only one yAccessor, the first part is not null | ||
if (hasSingleSeries || nameKeys.length === 0 || nameKeys[0] == null) { | ||
if (!spec) { | ||
return ''; | ||
} | ||
|
||
if (spec.splitSeriesAccessors && nameKeys.length > 0 && nameKeys[0] != null) { | ||
name = nameKeys.join(delimiter); | ||
} else { | ||
name = typeof spec.name === 'string' ? spec.name : `${spec.id}`; | ||
} | ||
} else { | ||
name = nameKeys.join(delimiter); | ||
} | ||
|
||
return name; | ||
const multipleYAccessors = spec && spec.yAccessors.length > 1; | ||
const nameKeys = multipleYAccessors ? seriesIdentifier.seriesKeys : seriesIdentifier.seriesKeys.slice(0, -1); | ||
const nonZeroLength = nameKeys.length > 0; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The block below ( |
||
return nonZeroLength && (spec?.splitSeriesAccessors || !hasSingleSeries) | ||
? nameKeys.join(typeof spec?.name === 'object' ? spec.name.delimiter ?? SERIES_DELIMITER : SERIES_DELIMITER) | ||
: spec === undefined | ||
? '' | ||
: typeof spec.name === 'string' | ||
? spec.name | ||
: spec.id; | ||
} | ||
|
||
function getSortIndex({ specSortIndex }: SeriesCollectionValue, total: number): number { | ||
|
@@ -612,45 +586,21 @@ export function getSortedDataSeriesColorsValuesMap( | |
|
||
/** | ||
* Helper function to get highest override color. | ||
* | ||
* from highest to lowest: `temporary`, `seriesSpec.color` then `persisted` | ||
* | ||
* @param key | ||
* @param customColors | ||
* @param overrides | ||
* From highest to lowest: `temporary`, `seriesSpec.color` then, unless `temporary` is set to `null`, `persisted` | ||
*/ | ||
function getHighestOverride( | ||
key: string, | ||
customColors: Map<SeriesKey, Color>, | ||
overrides: ColorOverrides, | ||
): Color | undefined { | ||
const tempColor: Color | undefined | null = overrides.temporary[key]; | ||
|
||
if (tempColor) { | ||
return tempColor; | ||
} | ||
|
||
const customColor: Color | undefined | null = customColors.get(key); | ||
|
||
if (customColor) { | ||
return customColor; | ||
} | ||
|
||
if (tempColor === null) { | ||
// Use default color when temporary and custom colors are null | ||
return; | ||
} | ||
|
||
return overrides.persisted[key]; | ||
// Unexpected empty `tempColor` string is falsy and falls through, see comment in `export type Color = ...` | ||
// Use default color when temporary and custom colors are null | ||
return tempColor || customColors.get(key) || (tempColor === null ? undefined : overrides.persisted[key]); | ||
nickofthyme marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
* Returns color for a series given all color hierarchies | ||
* | ||
* @param seriesCollection | ||
* @param chartColors | ||
* @param customColors | ||
* @param overrides | ||
* @internal | ||
*/ | ||
export function getSeriesColors( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏼