-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Visualize] Fix export table for table export links #71249
Conversation
53f7542
to
8940633
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Pinging @elastic/kibana-app (Team:KibanaApp) |
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.
code LGTM for app-arch changes
@dej611 thanx for taking care of this! Do you also want to update the table_vis_fn_test, add also a test title to the visConfig object? Also can you update the description with the issue that it closes? 🚀 |
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.
LGTM, tested in FF 🍪
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.
The next steps do not work for me:
- Export just saved visualization:
- create a new data table visualization;
- save it (do not left the page);
- export table - the
unsaved.csv
title is in use (expected the saved title)
- Export renamed visualization:
- open any saved visualization;
- re-save it with new name (do not left the page);
- click export - the previous saved title is in use (expected the new saved title)
@@ -33,6 +33,7 @@ export const visualization = () => ({ | |||
const visType = config.visType || visConfig.type; | |||
|
|||
const vis = new ExprVis({ | |||
title: config.title, |
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.
Seems to be unnecessary line
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.
It may be unnecessary for this case, but ExprVis
has a title
prop already configured, the title
position was not in the right location so I fixed while there.
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.
You are right, but the config
doesn't have a title in any kind of visualization.
The config is the result of fn
of a visualization (in case of table vis that's the result of table_vis_fn.ts
-> fn
function).
If you check configs of other visualizations, you'll see that any config doesn't have a title.
So this line do nothing
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.
LGTM, checked locally, works as expected
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Kibana Embedded in iframe with X-Pack Security.x-pack/test/functional_embedded/tests/iframe_embedded·ts.Kibana embedded in iframe should open Kibana for logged-in userStandard Out
Stack Trace
Build metrics
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* master: (21 commits) [Maps] 7.9 design improvements (elastic#71563) [ML] Changing all calls to ML endpoints to use internal user (elastic#70487) [eventLog] prevent log writing when initialization fails (elastic#71339) [Observability] landing page always being displayed (elastic#71494) [IM] Address data stream copy feedback (elastic#71615) [Logs UI] Anomalies page dataset filtering (elastic#71110) [data.search.aggs] Remove `use_field_mapping` from top hits agg (elastic#71168) [ML] Anomaly swim lane embeddable navigation and filter actions (elastic#71082) Fixes typo in siem_cloudtrail job description (elastic#71569) Require granted API Keys to have a name (elastic#71623) Update getUsageForCollection (elastic#71609) Only fetch saved elements once (elastic#71310) [SecuritySolution][Resolver] Adding siem index and guarding process ancestry (elastic#71570) [APM] Additional data telemetry changes (elastic#71112) [Visualize] Fix export table for table export links (elastic#71249) [Search] Server side search API (elastic#70446) use inclusive language (elastic#71607) [Security Solution] Hide timeline footer when Resolver is open (elastic#71516) [Index template wizard] Remove shadow and use border for components panels (elastic#71606) [ML] Kibana API endpoint for histogram chart data (elastic#70976) ...
Summary
This PR addresses a naming issue with the export link in the Visualize Data Table plugin.
The plugin itself contains some download logic, which was ignoring the table name since the 7.7.0 release.
In this PR:
unsaved
default name as the Inspector download actionsFixes #67289
Checklist