[NP] Inline buildPointSeriesData and buildHierarchicalData dependencies#61575
Merged
maryia-lapata merged 38 commits intoelastic:masterfrom Apr 9, 2020
Merged
[NP] Inline buildPointSeriesData and buildHierarchicalData dependencies#61575maryia-lapata merged 38 commits intoelastic:masterfrom
maryia-lapata merged 38 commits intoelastic:masterfrom
Conversation
Contributor
Author
|
@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 => { |
Contributor
Author
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; |
Contributor
Author
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
| label: yLabel == null ? id : yLabel, | ||
| count: 0, | ||
| id: id.split('-').pop() as string, | ||
| values: [point], |
Contributor
Author
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 { |
maryia-lapata
commented
Apr 3, 2020
| series: { accessor: group }, | ||
| }; | ||
| x: { accessor: x } as Dimension, | ||
| y: [{ accessor: y } as Dimension], |
Contributor
Author
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]; | |||
Contributor
Author
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; |
Contributor
Author
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; |
Contributor
Author
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)) { | |||
Contributor
|
In vislib something strange is happening with more complex charts (like This PR: Seems like the axis mapping and the dot config isn't working right. |
flash1293
reviewed
Apr 7, 2020
| export function getPoint( | ||
| table: Table, | ||
| x: Aspect, | ||
| series: Aspect[] | undefined, |
Contributor
Author
There was a problem hiding this comment.
flash1293
reviewed
Apr 7, 2020
Contributor
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; |
Contributor
There was a problem hiding this comment.
The cell in a table can also be an object (e.g. for date ranges):
Contributor
Author
There was a problem hiding this comment.
Nice catch! Thanks, I updated the PR.
| intervalESValue: number; | ||
| intervalESUnit: Unit; | ||
| format: string; | ||
| bounds?: { |
Contributor
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:
Contributor
Author
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) |
Contributor
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 = []; |
Contributor
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,
}))
});
Contributor
Author
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); | ||
| } |
Member
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
Contributor
flash1293
left a comment
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 = [ |
Contributor
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) { |
Contributor
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;| }, | ||
| parent: series ? series[0] : null, | ||
| }; | ||
| } as Point; |
Contributor
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.
Contributor
💛 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
Member
kertal
left a comment
There was a problem hiding this comment.
Code LGTM 👍 , tested locally with Chrome OS, MacOs 10.14.6, works, Дзякуй!
mbondyra
approved these changes
Apr 9, 2020
Contributor
mbondyra
left a comment
There was a problem hiding this comment.
Haven't tested, but went through the code and it LGTM! 💯
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>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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;