Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid #92442

DianaDerevyankina · 2021-02-23T16:04:04Z

Closes #59949

Summary

Replaced EuiCodeBlock with JsonCodeEditor in DiscoverGrid cell popover

To test:

In Advanced settings set the value of doc_table:legacy property to false
Open Discover
Click expand button on any cell in Document column

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

kertal

Thx for this migration! Is there an option to remove the line numbers here? Since I think they are not necessary in this case, and it would save a bit of screen space here. Also I'd like to have an opinion of our valued @elastic/kibana-design team about this change

…line numbers from the popover

myasonik · 2021-02-25T13:40:44Z

I guess it would depend on the specific use case on whether or not the line numbers are helpful.

Not sure that this is what we want to do, but to at least know what the possibilities are: is it possible to add a toggle in this popover to show/hide the line numbers?

DianaDerevyankina · 2021-03-01T07:23:05Z

@kertal what do you think about the toggle for displaying line numbers?

kertal · 2021-03-01T07:42:39Z

@kertal what do you think about the toggle for displaying line numbers?

I do agree that there might be situations when this is useful, but generally for me it's a quick preview and shouldn't have to much additional UI, dear @snide & @ryankeairns, wdyt?

ryankeairns · 2021-03-01T17:07:53Z

I side with @kertal . This is a rather small preview capability whereas a user can access a larger view of the JSON in the flyout by clicking on the expand icon at the front of each row.

Generally speaking, I wonder about the value for this small preview. Perhaps we should consider opening the flyout with the JSON tab opened.

kertal · 2021-03-02T07:46:00Z

Generally speaking, I wonder about the value for this small preview. Perhaps we should consider opening the flyout with the JSON tab opened.

This is not just a small preview for the all of _fields or _source, it's being used for any data structure that is of type object, so I wonder if it should behave differently in this case?

ryankeairns · 2021-03-02T15:00:41Z

Generally speaking, I wonder about the value for this small preview. Perhaps we should consider opening the flyout with the JSON tab opened.

This is not just a small preview for the all of _fields or _source, it's being used for any data structure that is of type object, so I wonder if it should behave differently in this case?

Ah, ok. For the non _fields or _source objects, is there any other way to expand or view that data or is the popover the only way?

kertal · 2021-03-02T15:17:26Z

Ah, ok. For the non _fields or _source objects, is there any other way to expand or view that data or is the popover the only way?

The popover is the only way to expand a single value, except of course they are part of the doc view table and JSON view.

ryankeairns · 2021-03-02T16:45:59Z

Thanks for the context @kertal . My line of thought here is to either include the line number or not (I lean towards not), but not invest much more in adding additional UI/options until we have a chance to think this through further. I sense there may be a path of leveraging the flyout in more cases, but that will take us running through a few use cases to verify that assumption.

PS - oops, I closed this on accident... speaking of tricky UX ;)

kertal · 2021-03-02T16:51:48Z

Thanks for the context @kertal . My line of thought here is to either include the line number or not (I lean towards not), but not invest much more in adding additional UI/options until we have a chance to think this through further. I sense there may be a path of leveraging the flyout in more cases, but that will take us running through a few use cases to verify that assumption.

PS - oops, I closed this on accident... speaking of tricky UX ;)

I also lead towards no line numbers, so I suggest going forward here (Think your unconsciousness wants also a close button in the popover, that might explain your choice of button ... suggesting a different issue for that ;)

elasticmachine · 2021-03-04T10:55:03Z

Pinging @elastic/kibana-app (Team:KibanaApp)

alexwizp · 2021-03-04T11:59:45Z

@elasticmachine merge upstream

VladLasitsa · 2021-03-05T13:26:08Z

src/plugins/discover/public/application/components/discover_grid/get_render_cell_value.tsx

@@ -108,8 +109,7 @@ export const getRenderCellValueFn = (

  if (field && field.type === '_source') {
    if (isDetails) {
-      // nicely formatted JSON for the expanded view
-      return <span>{JSON.stringify(row, null, 2)}</span>;
+      return <JsonCodeEditor hit={row} hasLineNumbers={false} width={370} />;


Could you please provide some details why 370 and maybe create constant for this?

nreese

maps changes LGTM
code review

kertal · 2021-03-08T15:26:41Z

@elasticmachine merge upstream

kertal

Tested locally in Safari, Firefox, Chrome, OSX, works like expected with the Documents column, but with other columns it currently doesn't work, for example geoip.location
OLD:

NEW:

kibanamachine · 2021-03-16T12:33:32Z

💚 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`	402.0KB	402.0KB	+50.0B

Page load bundle

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

id	before	after	diff
`discover`	79.6KB	81.1KB	+1.5KB

History

💚 Build #111754 succeeded d5d85fd
💚 Build #111378 succeeded 78d5b62
💔 Build #111371 failed 1c044c3
💚 Build #111023 succeeded 092dbbe
💚 Build #110388 succeeded c0ffe5b

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

cc @dziyanadzeraviankina

kertal

Code LGTM, tested locally in Chrome, Firefox, Safari, MacOS, works as expected, thx for the functional and the code simplification 🙏

stratoula · 2021-03-17T08:10:04Z

@elasticmachine run elasticsearch-ci/docs

* Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid * Add optional "hasLineNumbers" property to JsonCodeEditor and removed line numbers from the popover * Update json_code_editor snapshot * Add functional test for cell expanded content popover * Remove unused code * Fix geo point case and refactor some code Co-authored-by: Kibana Machine <[email protected]>

…4780) * Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid * Add optional "hasLineNumbers" property to JsonCodeEditor and removed line numbers from the popover * Update json_code_editor snapshot * Add functional test for cell expanded content popover * Remove unused code * Fix geo point case and refactor some code Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

…-action * 'master' of github.com:elastic/kibana: (44 commits) Migrate the optimizer mixin to core (#94272) Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid (#92442) [ML] Anomaly Detection: Migrate validation messages links to use docLinks. (#94568) [Lists][Exceptions] - Adding basic linting, i18n and storybook support (#94772) [Fleet] Add test/fix for invalid/missing ids in bulk agent reassign (#94632) [Security Solution] [Cases] Move create page components and dependencies to Cases (#94444) [ML] Data Frame Analytics accessibility tests: fix flaky outlier creation test (#94735) [Security Solutions] Remove commented out old linter rules (#94753) [App Search] Role mappings migration part 2 (#94461) [CI] Update Backport action inputs to match updated ones (#94721) [chore] Remove the infra team from CODEOWNERS (#94740) [Connectors UI] Make UI use new connector APIs (#94180) [ML] Use indices options in anomaly detection job wizards (#91830) Remove `string` as a valid registry var type value (#94174) [Alerts] Replaces legacy es client with the ElasticsearchClient for alerts and triggers_actions_ui plugins. (#93364) [Reporting-CSV Export] Re-write CSV Export using SearchSource (#88303) chore(NA): upgrade bazel rules nodejs to v3.2.2 (#94726) [APM] Settings: Update layout and update/add descriptions (#94398) skip flaky suite (#94666) [XY axis] Integrates legend color picker with the eui palette (#90589) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/constants.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_context.tsx # x-pack/plugins/index_lifecycle_management/public/shared_imports.ts

Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid

8902867

DianaDerevyankina added Feature:Discover Discover Application Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Feb 23, 2021

DianaDerevyankina requested review from VladLasitsa, alexwizp and sulemanof February 23, 2021 16:04

DianaDerevyankina self-assigned this Feb 23, 2021

alexwizp requested a review from kertal February 25, 2021 06:53

kertal reviewed Feb 25, 2021

View reviewed changes

kertal requested a review from a team February 25, 2021 10:35

DianaDerevyankina added 3 commits February 25, 2021 14:34

Merge branch 'master' into issues/59949-1

31a1801

Add optional "hasLineNumbers" property to JsonCodeEditor and removed …

e0cc318

…line numbers from the popover

Update json_code_editor snapshot

c0ffe5b

ryankeairns closed this Mar 2, 2021

ryankeairns reopened this Mar 2, 2021

DianaDerevyankina marked this pull request as ready for review March 4, 2021 10:55

DianaDerevyankina requested a review from a team March 4, 2021 10:55

kibanamachine and others added 2 commits March 4, 2021 06:59

Merge branch 'master' into issues/59949-1

092dbbe

Merge branch 'master' into issues/59949-1

09040ac

VladLasitsa reviewed Mar 5, 2021

View reviewed changes

Add functional test for cell expanded content popover

1c044c3

DianaDerevyankina requested a review from a team as a code owner March 5, 2021 13:45

Remove unused code

78d5b62

nreese approved these changes Mar 5, 2021

View reviewed changes

Merge branch 'master' into issues/59949-1

d5d85fd

kertal reviewed Mar 9, 2021

View reviewed changes

DianaDerevyankina added 2 commits March 16, 2021 13:14

Merge branch 'master' into issues/59949-1

e2e09e9

Fix geo point case and refactor some code

9ea6310

kertal approved these changes Mar 16, 2021

View reviewed changes

ryankeairns approved these changes Mar 16, 2021

View reviewed changes

DianaDerevyankina merged commit bdcd2ec into elastic:master Mar 17, 2021

DianaDerevyankina mentioned this pull request Mar 17, 2021

[7.x] Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid (#92442) #94780

Merged

kertal mentioned this pull request Jan 11, 2022

possibility to collapse blocks in JSON expanded documents in Discovery #122256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid #92442

Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid #92442

DianaDerevyankina commented Feb 23, 2021

kertal left a comment

myasonik commented Feb 25, 2021

DianaDerevyankina commented Mar 1, 2021

kertal commented Mar 1, 2021 •

edited

Loading

ryankeairns commented Mar 1, 2021

kertal commented Mar 2, 2021

ryankeairns commented Mar 2, 2021

kertal commented Mar 2, 2021

ryankeairns commented Mar 2, 2021

kertal commented Mar 2, 2021

elasticmachine commented Mar 4, 2021

alexwizp commented Mar 4, 2021

VladLasitsa Mar 5, 2021

nreese left a comment

kertal commented Mar 8, 2021

kertal left a comment

kibanamachine commented Mar 16, 2021

kertal left a comment

stratoula commented Mar 17, 2021

Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid #92442

Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid #92442

Conversation

DianaDerevyankina commented Feb 23, 2021

Summary

To test:

Checklist

For maintainers

kertal left a comment

Choose a reason for hiding this comment

myasonik commented Feb 25, 2021

DianaDerevyankina commented Mar 1, 2021

kertal commented Mar 1, 2021 • edited Loading

ryankeairns commented Mar 1, 2021

kertal commented Mar 2, 2021

ryankeairns commented Mar 2, 2021

kertal commented Mar 2, 2021

ryankeairns commented Mar 2, 2021

kertal commented Mar 2, 2021

elasticmachine commented Mar 4, 2021

alexwizp commented Mar 4, 2021

VladLasitsa Mar 5, 2021

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

kertal commented Mar 8, 2021

kertal left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 16, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

History

kertal left a comment

Choose a reason for hiding this comment

stratoula commented Mar 17, 2021

kertal commented Mar 1, 2021 •

edited

Loading