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] Improve HTML formatting of fields with a list of values #136684

Merged

Conversation

davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Jul 19, 2022

Summary

This PR improves HTML formatting of fields with a list of values to help differentiate between long numbers and numeric arrays. These changes were made:

  • Array values now include brackets in addition to commas.
  • Array brackets and commas are highlighted grey to make it clear they aren't part of the value.
  • For arrays that include values with newlines, the brackets are placed on their own lines and the array content is indented to improve appearance.

Discover grid:
table_view

Expanded document:
document_view

Resolves #135357.

Checklist

For maintainers

@davismcphee davismcphee self-assigned this Jul 19, 2022
@davismcphee davismcphee added release_note:enhancement Feature:Discover Discover Application Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) v8.4.0 labels Jul 19, 2022
@davismcphee davismcphee force-pushed the enhancement-discover-array-field-format branch from 6731fa3 to e6c388d Compare July 20, 2022 19:35
@davismcphee davismcphee force-pushed the enhancement-discover-array-field-format branch from e6c388d to ebebb66 Compare July 21, 2022 22:43
@davismcphee davismcphee requested a review from a team July 21, 2022 23:03
@davismcphee davismcphee marked this pull request as ready for review July 21, 2022 23:50
@davismcphee davismcphee requested review from a team as code owners July 21, 2022 23:50
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, this is a nice little big improvement , thank you 🙏 ... tested a-la-carte and works as expected. Just one note, the text in the PR doesn't mention what was changed (like added fancy brackets). this could be added

@davismcphee
Copy link
Contributor Author

Code LGTM, this is a nice little big improvement , thank you 🙏 ... tested a-la-carte and works as expected. Just one note, the text in the PR doesn't mention what was changed (like added fancy brackets). this could be added

Good call, summary updated to include changes.

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Nice improvement, LGTM.

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM,

I come up with this edge case, 3 values, but look like 5 (except subtle color difference in commas) :

Screenshot 2022-07-25 at 11 10 54

Screenshot 2022-07-25 at 11 10 34

But I don't think this is a blocker, since we had same edge case in main and overall changes makes formatting better

@kertal
Copy link
Member

kertal commented Jul 25, 2022

LGTM,

I come up with this edge case, 3 values, but look like 5 (except subtle color difference in commas) :

Screenshot 2022-07-25 at 11 10 54 Screenshot 2022-07-25 at 11 10 34

But I don't think this is a blocker, since we had same edge case in main and overall changes makes formatting better

this is a neat edge case @Dosant , also do agree it shouldn't block, but it's good to be aware of this 👍 . BTW there are no plans to add React based formatters which would e.g. allow expand/collapse such a list in UI, are there?

@kertal
Copy link
Member

kertal commented Jul 25, 2022

@elasticmachine merge upstream

@Dosant
Copy link
Contributor

Dosant commented Jul 25, 2022

there are no plans to add React based formatters which would e.g. allow expand/collapse such a list in UI, are there?

There are no plans, but there is this bug in the backlog: #75729, which I think could be fixed with some kind of React-based formatter where internally it updates with an interval

Another use-case I had in mind is that we possibly could use lazy-loading inside such React-based formatter and stop loading some of those heavy formatting functions into the initial bundle with all the formatters

I think it is worth at least creating an issue even if this won't be escalated soon

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #30 / machine learning - data visualizer index based actions panel on trial license view in discover page action loads the source data in the data visualizer

Metrics [docs]

Page load bundle

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

id before after diff
fieldFormats 45.0KB 45.3KB +291.0B

History

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

cc @davismcphee

@davismcphee
Copy link
Contributor Author

I come up with this edge case, 3 values, but look like 5 (except subtle color difference in commas) :

Oh yeah, good catch. I guess to the very keen eye the comma colours could help, but definitely not ideal. This will be good to keep in mind if we later go the React direction.

@davismcphee davismcphee merged commit a37aae2 into elastic:main Jul 25, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 25, 2022
@davismcphee davismcphee deleted the enhancement-discover-array-field-format branch July 25, 2022 15:30
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:enhancement Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing field formatting in Discover expanded doc fly-out
7 participants