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

[Discover] Fix document explorer cell popover rendering #123194

Conversation

kertal
Copy link
Member

@kertal kertal commented Jan 18, 2022

Summary

This PR fixes the way values are rendered in the cell popover when fetching using the fields API (which is the default). With this PR the JSON viewer is just shown when rendering the Document columns, and for rendering top level objects (like products of our ecommerce demo dataset) when fetching using _source. In all other cases the value the field formatter returns is displayed.

Before:
Bildschirmfoto 2022-01-18 um 12 12 20

After:
Bildschirmfoto 2022-01-18 um 12 08 54

Fixes #118162

Checklist

@kertal kertal self-assigned this Jan 18, 2022
@kertal kertal added Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) Feature:Discover Discover Application labels Jan 18, 2022
@kertal kertal added v8.1.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 18, 2022
@kertal kertal marked this pull request as ready for review January 18, 2022 11:13
@kertal kertal requested a review from a team as a code owner January 18, 2022 11:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@kertal
Copy link
Member Author

kertal commented Jan 19, 2022

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Jan 20, 2022

@elasticmachine merge upstream

@kertal
Copy link
Member Author

kertal commented Jan 24, 2022

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@kertal kertal requested a review from dimaanj January 25, 2022 15:33
@kertal
Copy link
Member Author

kertal commented Jan 26, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
discover 330.8KB 330.4KB -345.0B

History

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

cc @kertal

@kertal kertal requested a review from dimaanj January 26, 2022 15:10
Copy link
Contributor

@dimaanj dimaanj left a comment

Choose a reason for hiding this comment

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

Tested using a-la-carte. LTGM!

Copy link
Contributor

@majagrubic majagrubic left a comment

Choose a reason for hiding this comment

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

Tested this in Chrome on Mac OSX, works as expected. I don't fully understand where the motivation for the change is coming from, I feel the JSON view is nicer, but I trust you that this was the correct decision.

)
.join(', ');
const pairs = highlights[key] ? highlightPairs : sourcePairs;
if (displayKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I wrote this code, but omg is it horrible 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

so I've got good news for your, since I refactured the structure of the file, but did not touch the function's logic ... now technically: I wrote this code :)

@kertal kertal merged commit b1e3bdf into elastic:main Jan 27, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 27, 2022
awahab07 pushed a commit to awahab07/kibana that referenced this pull request Jan 31, 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:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Make values render in expanded popover formatted (and fix JSON)
6 participants