[NP] Inline buildPointSeriesData and buildHierarchicalData dependencies #61575
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maryia-lapata
added
v8.0.0
release_note:skip
|
@elasticmachine merge upstream |
maryia-lapata
commented
Apr 3, 2020
| */ | ||
| export function getAspects(table: Table, dimensions: Dimensions) { | ||
| const aspects: Aspects = {} as Aspects; | ||
| (Object.keys(dimensions) as Array<keyof Dimensions>).forEach(name => { |
There was a problem hiding this comment.
Since we know Discover's dimensions object exactly, I simplified all functions for Discover to get rid of unnecessary code.
maryia-lapata
commented
Apr 3, 2020
| } | ||
|
|
||
| const { y } = chart.aspects; | ||
| chart.yAxisLabel = y.length > 1 ? '' : y[0].title; |
There was a problem hiding this comment.
I didn't initialize yAxisFormat, since it isn't used in DiscoverHistogram component.
maryia-lapata
commented
Apr 3, 2020
| rawId: id, | ||
| label: yLabel == null ? id : yLabel, | ||
| count: 0, | ||
| id: id.split('-').pop() as string, | ||
| values: [point], |
There was a problem hiding this comment.
In the component the values is used only, so I removed all other fields.
maryia-lapata
commented
Apr 3, 2020
| }; | ||
| } | ||
|
|
||
| export interface Dimensions { |
There was a problem hiding this comment.
Here I based on this config structure.
maryia-lapata
commented
Apr 3, 2020
| series: { accessor: group }, | ||
| }; | ||
| x: { accessor: x } as Dimension, | ||
| y: [{ accessor: y } as Dimension], |
maryia-lapata
commented
Apr 3, 2020
| @@ -71,10 +107,9 @@ export function getPoint(table, x, series, yScale, row, rowIndex, y, z) { | |||
| } | |||
|
|
|||
| if (series) { | |||
| const seriesArray = series.length ? series : [series]; | |||
There was a problem hiding this comment.
series will be an array or undefined
maryia-lapata
commented
Apr 3, 2020
| const multiY = Array.isArray(aspects.y) && aspects.y.length > 1; | ||
| const yScale = chart.yScale; |
There was a problem hiding this comment.
Since chart object is created in point_series files and there is no initializing of yScale, I removed all code related to yScale
maryia-lapata
commented
Apr 3, 2020
|
|
||
| if (firstVal) { | ||
| y = _.find(aspects.y, function(y) { | ||
| return y.accessor === firstVal.accessor; |
There was a problem hiding this comment.
Here firstVal is an object with type Point, which doesn't have accessor property. It means that y will be undefined and an iteratee function will always return series.length => there's no sense in sortBy. That's why I removed this code.
maryia-lapata
commented
Apr 3, 2020
| @@ -28,12 +30,7 @@ export function initYAxis(chart) { | |||
|
|
|||
| const z = chart.aspects.series; | |||
| if (z) { | |||
| if (Array.isArray(z)) { | |||
There was a problem hiding this comment.
z is always be an array.
flash1293
reviewed
Apr 7, 2020
| export function getPoint( | ||
| table: Table, | ||
| x: Aspect, | ||
| series: Aspect[] | undefined, |
There was a problem hiding this comment.
flash1293
reviewed
Apr 7, 2020
There was a problem hiding this comment.
I tested around and couldn't find any broken things (besides the thing already reported separately). I left a few comments about typings. I think there is still room for improval beyond my comments, but we don't have to be 100% accurate here IMHO - if the things I commented on are working fine it feels to me like it's in a good shape.
Great work on the tests!
| return; | ||
| } | ||
| export interface Row { | ||
| [key: string]: number | string; |
There was a problem hiding this comment.
The cell in a table can also be an object (e.g. for date ranges):
There was a problem hiding this comment.
Nice catch! Thanks, I updated the PR.
| intervalESValue: number; | ||
| intervalESUnit: Unit; | ||
| format: string; | ||
| bounds?: { |
There was a problem hiding this comment.
Isn't bounds always set in discover when there is a histogram?
Also, it seems like the actual type of this is a moment instance:
There was a problem hiding this comment.
It seems so. Please take a look at 10eca3f.
| }; | ||
|
|
||
| if (bounds) { | ||
| chart.ordered.min = isNaN(bounds.min as number) |
There was a problem hiding this comment.
If the types from the comments above are fixed, then these type casts should go away.
# Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json
…ta/kibana into agg-response-cleanup
mbondyra
reviewed
Apr 8, 2020
|
|
||
| chart.yAxisLabel = table.columns[y.accessor].name; | ||
|
|
||
| chart.values = []; |
There was a problem hiding this comment.
2 nits: I am not sure why you're explicitly passing rowIndex below? Also, what do you think about replacing the below lines with a bit more functional code?
chart.values = table.rows
.filter(row => (row && row[yAccessor] !== 'NaN')
.map(row=>({
x: row[xAccessor] as number,
y: row[yAccessor] as number,
}))
});
There was a problem hiding this comment.
Nice catch! thanks, I updated the PR.
kertal
reviewed
Apr 9, 2020
Comment on lines
110
to
112
| if (!chart.values.length) { | ||
| chart.values.push({} as any); | ||
| } |
There was a problem hiding this comment.
I think you can easily remove this lines, by a modification of histogram.tsx
e.g. by using
const domainMin = data[0]?.x > domainStart ? domainStart : data[0]?.x;
you no longer need to push this empty object, and the histogram is displaying 'No data to display'
flash1293
approved these changes
Apr 9, 2020
There was a problem hiding this comment.
Tested and couldn't find any problems, LGTM I worked a bit on improving types (maryia-lapata#12), but I don't think another round of review is necessary after these are fixed.
| }); | ||
|
|
||
| it('properly formats series values', function() { | ||
| const seriesAspect = [ |
There was a problem hiding this comment.
This test isn't really testing whether the formatter gets applied correctly. Could you wrap the deserialize in the mock in a jest.fn and verify whether it got called with the expected param?
| const aspects = {}; | ||
| Object.keys(dimensions).forEach(name => { | ||
| const dimension = Array.isArray(dimensions[name]) ? dimensions[name] : [dimensions[name]]; | ||
| export function getAspects(table: Table, dimensions: Dimensions) { |
There was a problem hiding this comment.
If you change the types a little here you need less casting:
const aspects: Partial<Aspects> = {};
(Object.keys(dimensions) as Array<keyof Dimensions>).forEach(name => {
const dimension = dimensions[name];
const dimensionList = Array.isArray(dimension) ? dimension : [dimension];
dimensionList.forEach(d => {
if (!d) {
return;
}
const column = table.columns[d.accessor];
if (!column) {
return;
}
if (!aspects[name]) {
aspects[name] = [];
}
aspects[name]!.push({
accessor: column.id,
column: d.accessor,
title: column.name,
format: d.format,
params: d.params,
});
});
});
if (!aspects.x) {
aspects.x = [makeFakeXAspect()];
}
return aspects as Aspects;| @@ -62,7 +98,7 @@ export function getPoint(table, x, series, yScale, row, rowIndex, y, z) { | |||
| title: table.$parent.name, | |||
| }, | |||
| parent: series ? series[0] : null, | |||
| }; | |||
| } as Point; | |||
There was a problem hiding this comment.
This cast hides some problems - looks like RowValue could also be string or object. When you change linge 64 to const point: Point = {, you see the places that don't match.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/canvas/custom_elements·ts.Canvas app custom elements deletes custom element when promptedStandard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
kertal
approved these changes
Apr 9, 2020
mbondyra
approved these changes
Apr 9, 2020
jloleysens
added a commit
to jloleysens/kibana
that referenced
this pull request
Apr 9, 2020
…chore/put-all-xjson-together * 'master' of github.com:elastic/kibana: [EPM] Update UI copy to use `integration` (elastic#63077) [NP] Inline buildPointSeriesData and buildHierarchicalData dependencies (elastic#61575) [Maps] create NOT EXISTS filter for tooltip property with no value (elastic#62849) [Endpoint] Add link to Logs UI to the Host Details view (elastic#62852) [UI COPY] Fixes typo in max_shingle_size for search_as_you_type (elastic#63071) [APM] docs: add alerting examples for APM (elastic#62864) [EPM] Change PACKAGES_SAVED_OBJECT_TYPE id (elastic#62818) docs: fix rendering of bulleted list (elastic#62855) Exposed AddMessageVariables as separate component (elastic#63007) Add Data - Adding cloud reset password link to cloud instructions (elastic#62835) [ML] DF Analytics: update memory estimate after adding exclude fields (elastic#62850) [Table Vis] Fix visualization overflow (elastic#62630) [Endpoint][EPM] Endpoint depending on ingest manager to initialize (elastic#62871) [Remote clusters] Fix flaky jest tests (elastic#58768) [Discover] Hide time picker when an indexpattern without timefield is selected (elastic#62134) Move search source parsing and serializing to data (elastic#59919) [ML] Functional tests - stabilize typing in mml input (elastic#63091) [data.search.aggs]: Clean up TimeBuckets implementation (elastic#62123) [ML] Functional transform tests - stabilize source selection (elastic#63087) add embed flag to saved object url as well (elastic#62926) # Conflicts: # x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index.tsx
maryia-lapata
added a commit
that referenced
this pull request
Apr 10, 2020
…es (#61575) (#63145) * Move buildHierarchicalData to vislib * Move shortened version of buildPointSeriesData to Discover * Move buildPointSeriesData to vis_type_vislib * Convert unit tests to jest * Remove ui/agg_response * Convert point_series files to TS * Update TS in unit tests * Convert buildHierarchicalData to TS * Convert buildPointSeriesData to TS in Discover * Clean TS in Discover * Update TS for buildHierarchicalData * Update buildHierarchicalData unit tests * Clean up TS in point_series * Add unit tests fro response_handler.js * Simplify point_series for Discover * Return array for data * Add check for empty row * Simplify point_series for Discover * Return all points * Specify TS * Refactoring * Simplifying * improve types * Update _get_point.test.ts Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Joe Reuter <johannes.reuter@elastic.co> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Joe Reuter <johannes.reuter@elastic.co>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #60090.
This PR contains:
buildHierarchicalDatatovis_type_vislib;buildPointSeriesDatatovis_type_vislib;buildPointSeriesDatatovis_type_visliband converting them to Jest;buildPointSeriesDatatodiscover;ui/agg_response;buildPointSeriesDataandbuildHierarchicalDatato TS;