Skip to content
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

[XY] Support vis_dimension type for accessors #128063

Conversation

VladLasitsa
Copy link
Contributor

Completes part of #127115

Summary

Added full support of visdimension at fields: splitAccessor, accessors, xAccessor, forAccessor

@VladLasitsa VladLasitsa self-assigned this Mar 29, 2022
@VladLasitsa VladLasitsa added Feature:XYAxis XY-Axis charts (bar, area, line) Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.2.0 labels Mar 29, 2022
mbondyra and others added 2 commits March 29, 2022 20:35
…pressions-xy-support-visdimension-type

# Conflicts:
#	packages/kbn-optimizer/limits.yml
#	src/plugins/chart_expressions/expression_xy/public/components/reference_lines.tsx
@VladLasitsa VladLasitsa marked this pull request as ready for review March 31, 2022 11:50
@VladLasitsa VladLasitsa requested a review from a team as a code owner March 31, 2022 11:50
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First glance at the code: LGTM 👍 Left a couple of nits.
I'll test locally tomorrow.

@@ -23,7 +27,9 @@ export const getAppliedTimeRange = (layers: DataLayerArgs[], data: LensMultiTabl
return Object.entries(data.tables)
.map(([tableId, table]) => {
const layer = layers.find((l) => l.layerId === tableId);
const xColumn = table.columns.find((col) => col.id === layer?.xAccessor);
const xColumn = layer?.xAccessor
? getColumnByAccessor(layer?.xAccessor, table.columns)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: you can use layer.xAccessor here.

.flatMap((layer) =>
data.tables[layer.layerId].rows.map((row) => row[layer.xAccessor!].valueOf() as number)
)
.flatMap((layer) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: instead of using as number, you could use flatMap<number> here.
PS: I know, you are not the author of this code.

@Kuznietsov
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
lens 467 471 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
expressionXY 42.7KB 43.9KB +1.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
expressionXY 23.9KB 25.2KB +1.3KB
Unknown metric groups

API count

id before after diff
lens 542 546 +4

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @VladLasitsa

Copy link
Contributor

@Kuznietsov Kuznietsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found such a weird bug. Could you, please, address it.

Screen.Recording.2022-04-01.at.16.59.49.mov

Expression:

kibana
| lens_merge_tables layerIds="f6ed53d9-fc5d-4051-85c1-f6a3eba00726"
  layerIds="8975a4f4-46c0-4481-9c91-1c036feb489d" 
  tables={esaggs index={indexPatternLoad id="90943e30-9a47-11e8-b64d-95841ca0b247"} aggs={aggDateHistogram id="0" enabled=true schema="segment" field="@timestamp" useNormalizedEsInterval=true interval="auto" drop_partials=false min_doc_count=0 extended_bounds={extendedBounds} extendToTimeRange=true} aggs={aggAvg id="1" enabled=true schema="metric" field="bytes"} metricsAtAllLevels=false partialRows=false timeFields="@timestamp" | lens_rename_columns idMap="{\"col-0-0\":{\"label\":\"@timestamp\",\"dataType\":\"date\",\"operationType\":\"date_histogram\",\"sourceField\":\"@timestamp\",\"isBucketed\":true,\"scale\":\"interval\",\"params\":{\"interval\":\"auto\",\"includeEmptyRows\":true,\"dropPartials\":false},\"id\":\"c2305c94-6b9e-4e8d-a46a-c10b79c76f01\"},\"col-1-1\":{\"label\":\"Average of bytes\",\"dataType\":\"number\",\"operationType\":\"average\",\"sourceField\":\"bytes\",\"isBucketed\":false,\"scale\":\"ratio\",\"params\":{\"emptyAsNull\":true},\"id\":\"4bca57f0-7d03-4fb3-b7a7-e1b6f418eac9\"}}"}
  tables={createTable   rowCount=1 | mathColumn id="26f34410-a1f5-42eb-bc10-6dc4bc893ac4" name="Static value: 7299" expression="7299"}
| xyVis title="" description="" xTitle="" yTitle="" yRightTitle="" legend={legendConfig isVisible=true  position="right"       shouldTruncate=true} fittingFunction="None" endValue="None" emphasizeFitting=false curveType="LINEAR" fillOpacity=0.3 yLeftExtent={axisExtentConfig mode="full"} yRightExtent={axisExtentConfig mode="full"} 
  axisTitlesVisibilitySettings={axisTitlesVisibilityConfig x=true yLeft=true yRight=true} tickLabelsVisibilitySettings={tickLabelsConfig x=true yLeft=true yRight=true} gridlinesVisibilitySettings={gridlinesConfig x=true yLeft=true yRight=true} labelsOrientation={labelsOrientationConfig x=0 yLeft=0 yRight=0} valueLabels="hide" hideEndzones=false valuesInLegend=false 
  layers={dataLayer layerId="f6ed53d9-fc5d-4051-85c1-f6a3eba00726" hide=false xAccessor="c2305c94-6b9e-4e8d-a46a-c10b79c76f01" yScaleType="linear" xScaleType="time" isHistogram=true   seriesType="bar_stacked" accessors={visdimension "4bca57f0-7d03-4fb3-b7a7-e1b6f418eac9"} columnToLabel="{\"4bca57f0-7d03-4fb3-b7a7-e1b6f418eac9\":\"Average of bytes\"}" palette={system_palette name="default"}}
  layers={referenceLineLayer layerId="8975a4f4-46c0-4481-9c91-1c036feb489d" yConfig={yConfig forAccessor="26f34410-a1f5-42eb-bc10-6dc4bc893ac4" axisMode="left" color="#69707d" lineStyle="solid" lineWidth=1 fill="none"  iconPosition="auto" textVisibility=false} accessors={visdimension "26f34410-a1f5-42eb-bc10-6dc4bc893ac4"} columnToLabel="{\"26f34410-a1f5-42eb-bc10-6dc4bc893ac4\":\"Static value: 7299\"}"}

The same about forAccessor:
Screenshot 2022-04-01 at 17 04 25

@VladLasitsa
Copy link
Contributor Author

As we have a lot of changes related to that PR: #128255 and it should be merged first. I close this and open another PR for that changes #129612 based on changes from here #128255.

@VladLasitsa VladLasitsa closed this Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:XYAxis XY-Axis charts (bar, area, line) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants