[Discover][DocViewer] Limit the height of long field values by default #183736

jughosta · 2024-05-17T12:48:07Z

Closes Bring back "toggle field details" in discover document view #181568

Summary

This PR adds a default height limit for values in DocViewer. If the value is too long, we visually truncate it and add "View more" button which allows to expand to view the entire value. This way all fields in the flyout are easily accessible (less scrolling required) even if they contain long values. ~~The height can be configured via the existing truncate:maxHeight Advanced Setting.~~ If user expands a value, closes the flyout and opens it again, the value will be shown as expanded again for that field.

Related: #164236

Testing

Some cases to check while testing:

varios value formats
legacy table vs data grid
doc viewer flyout vs Single Document page

Sample long value:

POST test_this/_doc/
{
  "message": """javax.servlet.ServletException: Something bad happened
    at com.example.myproject.OpenSessionInViewFilter.doFilter(OpenSessionInViewFilter.java:60)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at com.example.myproject.ExceptionHandlerFilter.doFilter(ExceptionHandlerFilter.java:28)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at com.example.myproject.OutputBufferFilter.doFilter(OutputBufferFilter.java:33)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1157)
    at org.mortbay.jetty.servlet.ServletHandler.handle(ServletHandler.java:388)
    at org.mortbay.jetty.security.SecurityHandler.handle(SecurityHandler.java:216)
    at org.mortbay.jetty.servlet.SessionHandler.handle(SessionHandler.java:182)
    at org.mortbay.jetty.handler.ContextHandler.handle(ContextHandler.java:765)
    at org.mortbay.jetty.webapp.WebAppContext.handle(WebAppContext.java:418)
    at org.mortbay.jetty.handler.HandlerWrapper.handle(HandlerWrapper.java:152)
    at org.mortbay.jetty.Server.handle(Server.java:326)
    at org.mortbay.jetty.HttpConnection.handleRequest(HttpConnection.java:542)
    at org.mortbay.jetty.HttpConnection$RequestHandler.content(HttpConnection.java:943)
    at org.mortbay.jetty.HttpParser.parseNext(HttpParser.java:756)
    at org.mortbay.jetty.HttpParser.parseAvailable(HttpParser.java:218)
    at org.mortbay.jetty.HttpConnection.handle(HttpConnection.java:404)
    at org.mortbay.jetty.bio.SocketConnector$Connection.run(SocketConnector.java:228)
    at org.mortbay.thread.QueuedThreadPool$PoolThread.run(QueuedThreadPool.java:582)
Caused by: com.example.myproject.MyProjectServletException
    at com.example.myproject.MyServlet.doPost(MyServlet.java:169)
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:727)
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:820)
    at org.mortbay.jetty.servlet.ServletHolder.handle(ServletHolder.java:511)
    at org.mortbay.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1166)
    at com.example.myproject.OpenSessionInViewFilter.doFilter(OpenSessionInViewFilter.java:30)
    ... 27 more
Caused by: org.hibernate.exception.ConstraintViolationException: could not insert: [com.example.myproject.MyEntity]
    at org.hibernate.exception.SQLStateConverter.convert(SQLStateConverter.java:96)
    at org.hibernate.exception.JDBCExceptionHelper.convert(JDBCExceptionHelper.java:66)
    at org.hibernate.id.insert.AbstractSelectingDelegate.performInsert(AbstractSelectingDelegate.java:64)
    at org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:2329)
    at org.hibernate.persister.entity.AbstractEntityPersister.insert(AbstractEntityPersister.java:2822)
    at org.hibernate.action.EntityIdentityInsertAction.execute(EntityIdentityInsertAction.java:71)
    at org.hibernate.engine.ActionQueue.execute(ActionQueue.java:268)
    at org.hibernate.event.def.AbstractSaveEventListener.performSaveOrReplicate(AbstractSaveEventListener.java:321)
    at org.hibernate.event.def.AbstractSaveEventListener.performSave(AbstractSaveEventListener.java:204)
    at org.hibernate.event.def.AbstractSaveEventListener.saveWithGeneratedId(AbstractSaveEventListener.java:130)
    at org.hibernate.event.def.DefaultSaveOrUpdateEventListener.saveWithGeneratedOrRequestedId(DefaultSaveOrUpdateEventListener.java:210)
    at org.hibernate.event.def.DefaultSaveEventListener.saveWithGeneratedOrRequestedId(DefaultSaveEventListener.java:56)
    at org.hibernate.event.def.DefaultSaveOrUpdateEventListener.entityIsTransient(DefaultSaveOrUpdateEventListener.java:195)
    at org.hibernate.event.def.DefaultSaveEventListener.performSaveOrUpdate(DefaultSaveEventListener.java:50)
    at org.hibernate.event.def.DefaultSaveOrUpdateEventListener.onSaveOrUpdate(DefaultSaveOrUpdateEventListener.java:93)
    at org.hibernate.impl.SessionImpl.fireSave(SessionImpl.java:705)
    at org.hibernate.impl.SessionImpl.save(SessionImpl.java:693)
    at org.hibernate.impl.SessionImpl.save(SessionImpl.java:689)
    at sun.reflect.GeneratedMethodAccessor5.invoke(Unknown Source)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.hibernate.context.ThreadLocalSessionContext$TransactionProtectionWrapper.invoke(ThreadLocalSessionContext.java:344)
    at $Proxy19.save(Unknown Source)
    at com.example.myproject.MyEntityService.save(MyEntityService.java:59) <-- relevant call (see notes below)
    at com.example.myproject.MyServlet.doPost(MyServlet.java:164)
    ... 32 more
Caused by: java.sql.SQLException: Violation of unique constraint MY_ENTITY_UK_1: duplicate value(s) for column(s) MY_COLUMN in statement [...]
    at org.hsqldb.jdbc.Util.throwError(Unknown Source)
    at org.hsqldb.jdbc.jdbcPreparedStatement.executeUpdate(Unknown Source)
    at com.mchange.v2.c3p0.impl.NewProxyPreparedStatement.executeUpdate(NewProxyPreparedStatement.java:105)
    at org.hibernate.id.insert.AbstractSelectingDelegate.performInsert(AbstractSelectingDelegate.java:57)
    ... 54 more"""
}

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

jughosta · 2024-05-17T14:23:28Z

/ci

elasticmachine · 2024-05-17T18:25:23Z

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

davismcphee

I haven't tested yet, and I think we definitely need something like this back in the doc viewer for long values, but two thoughts:

If we intend to migrate doc viewer to EUI data grid, will this be as useful since users will be able to customize the row height anyway?
With the design in this PR, if a user clicks "view more" and the value turns out to be really long, they have to scroll all the way to the end to get to the "view less" button, which seems like it could be a frustrating UX. Something we might want to consider from the design side.

jughosta · 2024-05-21T07:12:04Z

Hi @davismcphee,

If we intend to migrate doc viewer to EUI data grid, will this be as useful since users will be able to customize the row height anyway?

I think it would be still useful. EuiDataGrid allows to truncate all cell values or to auto-show all cell values but there is no option "in between" to show one value while truncating the others for example.

With the design in this PR, if a user clicks "view more" and the value turns out to be really long, they have to scroll all the way to the end to get to the "view less" button, which seems like it could be a frustrating UX. Something we might want to consider from the design side.

Good point. We could also return the arrow button which would be shown above the value.

kertal · 2024-05-21T09:21:48Z

Hi @davismcphee,

If we intend to migrate doc viewer to EUI data grid, will this be as useful since users will be able to customize the row height anyway?

I think it would be still useful. EuiDataGrid allows to truncate all cell values or to auto-show all cell values but there is no option "in between" to show one value while truncating the others for example.

I do agree at the current state I think it is useful at this state and also initially when introducing EuiDataGrid.

With the design in this PR, if a user clicks "view more" and the value turns out to be really long, they have to scroll all the way to the end to get to the "view less" button, which seems like it could be a frustrating UX. Something we might want to consider from the design side.

Good point. We could also return the arrow button which would be shown above the value.

Definitely good point! Visually I like the text buttons more than the expand error, but functional-wise, yes it could lead to lots of scrolling. Wdyt @andreadelrio @isaclfreire , back in the days, be fore we removed it it looked like this:

jughosta · 2024-06-03T14:25:50Z

@kertal @andreadelrio @davismcphee How should we proceed here? By returning the arrow button?

davismcphee · 2024-06-05T02:45:21Z

@jughosta It looks like @andreadelrio is on PTO this week, and it would be great to get her input if we can wait until she's back. Otherwise we could reach out to someone else from design instead.

andreadelrio · 2024-06-10T01:26:43Z

to migrate doc viewer to EUI data grid, will this be as useful since users will be able to customize the row height anyway?

Hi team, what if we do something like this? It's borrowing from the proposal I shared when we were going to do #179189. If we choose this direction, I'm happy to add the new icons (plus and minus in square) to EUI.

davismcphee · 2024-06-11T00:41:34Z

@andreadelrio that works for me!

kertal · 2024-06-11T07:28:10Z

@andreadelrio yes, much better than the old expand arrow

…-values # Conflicts: # src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table.scss

kertal · 2024-07-04T10:42:31Z

In the latest iteration @jughosta suggested an existing plus icon, since the new one in not yet available. For consistency of expanding I'd suggest to go with the arrow like back in the days, because it's also used in the field list. We can easily change to the new icons @andreadelrio suggested. I think with placing the arrow on the left side of the text if screen space allows it, it's already looking better. When testing I've noticed that Chrome / Firefox might need a bit less height, at least in my used example it looked like this

jughosta · 2024-08-02T11:09:30Z

/ci

… collapse-long-field-values

jughosta · 2024-08-02T12:28:53Z

/ci

jughosta · 2024-08-02T13:57:43Z

Ready for review 💫

andreadelrio

Design changes LGTM. Nice job @jughosta !

davismcphee

The code changes look good and it works well! Just left a couple of minor questions.

src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table_cell_value.test.tsx

davismcphee · 2024-08-09T02:55:45Z

src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table_cell_value.tsx

+  const { uiSettings } = getUnifiedDocViewerServices();
+  let truncateMaxHeight = uiSettings.get(TRUNCATE_MAX_HEIGHT);
+
+  if (truncateMaxHeight === TRUNCATE_MAX_HEIGHT_DEFAULT_VALUE) {


Rather than piggybacking off the existing legacy doc table max height, I wonder if we shouldn't just introduce a dedicated setting for the doc viewer flyout to avoid this hidden adjustment which seems a bit magical? Adding a new advanced setting isn't great, but ideally we'll be able to get rid of the legacy grid ones soon anyway.

Okay, agree with your concern. How about making it a fixed configuration in code without introducing another setting and mark the existing one as deprecated?

Yes let's do that 👍 advanced settings suck anyway so not introducing another is good. I thought about it a bit more too and I think it makes more sense to have this as a user/browser level setting instead if needed, like the ES|QL "hide null values" toggle, so we could discuss it with a designer as a potential followup.

davismcphee · 2024-08-09T02:56:59Z

src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table_cell_value.tsx

+  const isCollapsible =
+    !isDetails &&
+    truncateMaxHeight > 0 &&
+    String(rawValue).length > COLLAPSE_LINE_LENGTH &&


Do we need COLLAPSE_LINE_LENGTH if we're using the div height anyway? Couldn't we base it off only div height instead?

Right, I updated the implementation.

…ble/table_cell_value.test.tsx Co-authored-by: Davis McPhee <[email protected]>

…-values # Conflicts: # src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table.tsx

davismcphee

Another quick test and it's still working great 🙌 much cleaner looking than the old version too, thanks!

kibana-ci · 2024-08-10T09:06:10Z

💚 Build Succeeded

Buildkite Build
Commit: 7c25efe

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`unifiedDocViewer`	278	279	+1

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
`@kbn/discover-utils`	104	105	+1

Async chunks

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

id	before	after	diff
`unifiedDocViewer`	202.8KB	204.2KB	+1.4KB

Page load bundle

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

id	before	after	diff
`unifiedDocViewer`	12.0KB	12.0KB	+57.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/discover-utils`	130	131	+1

ESLint disabled line counts

id	before	after	diff
`unifiedDocViewer`	10	9	-1

Total ESLint disabled count

id	before	after	diff
`unifiedDocViewer`	10	9	-1

History

💛 Build #226871 was flaky efa74d6
💛 Build #226800 was flaky 4eb2b2b
💛 Build #226428 was flaky 0b69204
💛 Build #226324 was flaky a1e1187
💚 Build #225747 succeeded 5586997
💛 Build #225572 was flaky b1b40cf

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

cc @jughosta

- Closes #167582 ## Summary This PR removes the code related to the legacy doc table and 2 Advanced Settings: `doc_table:legacy` and `truncate:maxHeight`. The legacy table in Discover was replaced by the new data grid in v8.3. The `doc_table:legacy` Advanced Setting was added to let users switch back to the legacy table if necessary. The removal of the setting and the legacy table entirely would allow us to reduce bundle size, maintenance burden, and code complexity. Also the legacy table does not support many new features which were added to the grid only (e.g. comparing selected documents, context-aware UI based on current solution project, column resizing, bulk row selection, copy actions, new doc viewer flyout, and more). Since v8.15 `doc_table:legacy` is marked as deprecated on Advanced Settings page via #179899 Since v8.16 `truncate:maxHeight` is marked as deprecated too via #183736 The removal of these 2 settings and the associated code is planned for v9. ### Checklist - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

- Closes elastic#167582 ## Summary This PR removes the code related to the legacy doc table and 2 Advanced Settings: `doc_table:legacy` and `truncate:maxHeight`. The legacy table in Discover was replaced by the new data grid in v8.3. The `doc_table:legacy` Advanced Setting was added to let users switch back to the legacy table if necessary. The removal of the setting and the legacy table entirely would allow us to reduce bundle size, maintenance burden, and code complexity. Also the legacy table does not support many new features which were added to the grid only (e.g. comparing selected documents, context-aware UI based on current solution project, column resizing, bulk row selection, copy actions, new doc viewer flyout, and more). Since v8.15 `doc_table:legacy` is marked as deprecated on Advanced Settings page via elastic#179899 Since v8.16 `truncate:maxHeight` is marked as deprecated too via elastic#183736 The removal of these 2 settings and the associated code is planned for v9. ### Checklist - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

jughosta added 2 commits May 17, 2024 14:39

[Discover][DocViewer] Reintroduce limiting of long values by default

16613fd

[Discover][DocViewer] Clean up

f845d26

jughosta self-assigned this May 17, 2024

jughosta added 4 commits May 17, 2024 15:45

[Discover][DocViewer] Improve the logic

25a891f

[Discover][DocViewer] Update test

bb13aaa

[Discover][DocViewer] Rename

bec50e8

[Discover][DocViewer] Keep in memory expanded values

935974e

jughosta marked this pull request as ready for review May 17, 2024 18:25

jughosta requested review from a team as code owners May 17, 2024 18:25

davismcphee reviewed May 20, 2024

View reviewed changes

Merge branch 'main' into collapse-long-field-values

bd8cbd0

Merge branch 'main' into collapse-long-field-values

9386cfd

jughosta added 2 commits July 3, 2024 14:43

Merge remote-tracking branch 'upstream/main' into collapse-long-field…

ace9b68

…-values # Conflicts: # src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table.scss

[Discover] Switch to an icon button

0c74b1e

[Discover] Update icons and the overflow shadow

dcac57a

jughosta added 2 commits August 2, 2024 13:08

[Discover] Improve tests

00aef88

Merge branch 'main' into collapse-long-field-values

2ce432e

jughosta added 2 commits August 2, 2024 14:27

[Discover] Fix types

2a9539d

Merge remote-tracking branch 'origin/collapse-long-field-values' into…

b1b40cf

… collapse-long-field-values

jughosta marked this pull request as ready for review August 2, 2024 13:57

jughosta requested a review from a team August 2, 2024 13:58

jughosta added 2 commits August 5, 2024 09:14

Merge branch 'main' into collapse-long-field-values

5586997

Merge branch 'main' into collapse-long-field-values

a1e1187

andreadelrio approved these changes Aug 7, 2024

View reviewed changes

Merge branch 'main' into collapse-long-field-values

0b69204

davismcphee reviewed Aug 9, 2024

View reviewed changes

jughosta and others added 5 commits August 9, 2024 14:52

Merge branch 'main' into collapse-long-field-values

2d0a4e5

Update src/plugins/unified_doc_viewer/public/components/doc_viewer_ta…

4eb2b2b

…ble/table_cell_value.test.tsx Co-authored-by: Davis McPhee <[email protected]>

[Discover] Refactor the truncation to be based on the cell height

5a77b80

Merge remote-tracking branch 'upstream/main' into collapse-long-field…

f349d4f

…-values # Conflicts: # src/plugins/unified_doc_viewer/public/components/doc_viewer_table/table.tsx

[Discover] Fix updates

efa74d6

davismcphee approved these changes Aug 9, 2024

View reviewed changes

Merge branch 'main' into collapse-long-field-values

7c25efe

jughosta enabled auto-merge (squash) August 10, 2024 07:45

jughosta merged commit 625e89e into elastic:main Aug 10, 2024
19 checks passed

kibanamachine added the v8.16.0 label Aug 10, 2024

jughosta mentioned this pull request Nov 25, 2024

[Discover] Remove the legacy table #201254

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover][DocViewer] Limit the height of long field values by default #183736

[Discover][DocViewer] Limit the height of long field values by default #183736

jughosta commented May 17, 2024 •

edited

Loading

jughosta commented May 17, 2024

elasticmachine commented May 17, 2024

davismcphee left a comment

jughosta commented May 21, 2024

kertal commented May 21, 2024

jughosta commented Jun 3, 2024

davismcphee commented Jun 5, 2024

andreadelrio commented Jun 10, 2024

davismcphee commented Jun 11, 2024

kertal commented Jun 11, 2024

kertal commented Jul 4, 2024

jughosta commented Aug 2, 2024

jughosta commented Aug 2, 2024

jughosta commented Aug 2, 2024

andreadelrio left a comment

davismcphee left a comment

davismcphee Aug 9, 2024

jughosta Aug 9, 2024

davismcphee Aug 9, 2024

davismcphee Aug 9, 2024

jughosta Aug 9, 2024

davismcphee left a comment

kibana-ci commented Aug 10, 2024

API count

ESLint disabled line counts

Total ESLint disabled count

[Discover][DocViewer] Limit the height of long field values by default #183736

[Discover][DocViewer] Limit the height of long field values by default #183736

Conversation

jughosta commented May 17, 2024 • edited Loading

Summary

Testing

Checklist

jughosta commented May 17, 2024

elasticmachine commented May 17, 2024

davismcphee left a comment

Choose a reason for hiding this comment

jughosta commented May 21, 2024

kertal commented May 21, 2024

jughosta commented Jun 3, 2024

davismcphee commented Jun 5, 2024

andreadelrio commented Jun 10, 2024

davismcphee commented Jun 11, 2024

kertal commented Jun 11, 2024

kertal commented Jul 4, 2024

jughosta commented Aug 2, 2024

jughosta commented Aug 2, 2024

jughosta commented Aug 2, 2024

andreadelrio left a comment

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Aug 9, 2024

Choose a reason for hiding this comment

jughosta Aug 9, 2024

Choose a reason for hiding this comment

davismcphee Aug 9, 2024

Choose a reason for hiding this comment

davismcphee Aug 9, 2024

Choose a reason for hiding this comment

jughosta Aug 9, 2024

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 10, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

jughosta commented May 17, 2024 •

edited

Loading