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 document explorer flyout #120116

Merged
merged 23 commits into from
Dec 22, 2021

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Dec 1, 2021

Summary

Closes #104339

This PR improves UI for document explorer and single doc view and adds:

  • search for field names
  • pagination
  • ability to pin fields (except single doc view).

Previous state of pinned fields will be persisted per each data view. On single document view pinned fields will not be displayed. Previous search text and page size setting will also be persisted.

From performance perspective, with this changes described above now it's much faster to navigate through a large number of fields using search, pagination. Pinned fields always visible on the top of the table.

81C79578-657C-4621-8330-E4A5076FBC1B

Checklist

@dimaanj dimaanj added release_note:enhancement Feature:Discover Discover Application v8.1.0 Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Dec 1, 2021
@dimaanj dimaanj self-assigned this Dec 1, 2021
@andreadelrio
Copy link
Contributor

I would suggest that we try grouping the available actions (now 5) into a centralized "actions" button
Frame 23

Ideally the actions column would come last (that's the location it gets in the EuiTable and EuiDataGrid examples).
Frame 24

@kertal
Copy link
Member

kertal commented Dec 2, 2021

It's a nice suggestion. Think we should display them first, because last time we changed position we had to roll it back. (for people with very large screens)

I would suggest that we try grouping the available actions (now 5) into a centralized "actions" button Frame 23

…-document-explorer-sidebar

# Conflicts:
#	src/plugins/discover/public/services/doc_views/components/doc_viewer_table/legacy/table.test.tsx
#	src/plugins/discover/public/services/doc_views/components/doc_viewer_table/table.tsx
#	src/plugins/discover/public/services/doc_views/doc_views_types.ts
@dimaanj dimaanj marked this pull request as ready for review December 6, 2021 12:15
@dimaanj dimaanj requested review from a team as code owners December 6, 2021 12:15
@elasticmachine
Copy link
Contributor

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

@andreadelrio
Copy link
Contributor

@dmitriynj I've been trying to get this running locally to review but no luck so far. Can you please post an image of how this menu looks when a field has been pinned and the user clicks on the menu?

@kertal
Copy link
Member

kertal commented Dec 7, 2021

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Dec 7, 2021

also the a-la-carte deployment fails due to Error: Cannot find module '@kbn/apm-config-loader' that's odd

@majagrubic
Copy link
Contributor

@andreadelrio @dmitriynj I like grouping of the actions, but I don't like the current icon - it feels like something is still loading and we're waiting on results. any chance we can choose a different icon?

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 7, 2021

Can you please post an image of how this menu looks when a field has been pinned and the user clicks on the menu?

9193B2EB-0997-41D9-894D-975022A60AC6_1_105_c

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 7, 2021

Any chance we can choose a different icon?

One option I see is color primary.

ED9F671E-BDB6-4FB2-BDDF-1C92766C12F3_4_5005_c

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.

Love it, having search and pinning is very useful, here are my first remarks while playing with it:
Discover_-_Elastic

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 15, 2021

@elasticmachine merge upstream

@dimaanj dimaanj requested a review from kertal December 15, 2021 13:42
@kertal
Copy link
Member

kertal commented Dec 16, 2021

@elasticmachine merge upstream

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 20, 2021

@elasticmachine merge upstream

@kertal kertal added this to PRs in Discover via automation Dec 20, 2021
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.

While testing I noticed a strange behavior in Firefox: Pinning of field seems doesn't seem to be persisted? In Chrome and Safari it works great.

Screen.Recording.2021-12-20.at.12.27.50.PM.mp4

BTW: I'd suggest setting the title to [Discover] Improve document explorer flyout and mention the performance improvements when having a large field number + search + pinning in the description.

@dimaanj dimaanj changed the title [Discover] Improve document explorer sidebar performance [Discover] Improve document explorer flyout Dec 20, 2021
@kertal
Copy link
Member

kertal commented Dec 21, 2021

While testing I noticed a strange behavior in Firefox: Pinning of field seems doesn't seem to be persisted? In Chrome and Safari it works great.
Ok, now I know why it didn't work. in local storage, Firefox had the following value stored:

Bildschirmfoto 2021-12-21 um 13 30 17

this was returned and decoded as a string, that's why updating didn't work. I guess the implementation of the storage changed during the implementation? that's why it didn't work with 'legacy' stored value?

@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 21, 2021

Ok, now I know why it didn't work. in local storage, Firefox had the following value stored:

Bildschirmfoto 2021-12-21 um 13 30 17

this was returned and decoded as a string, that's why updating didn't work. I guess the implementation of the storage changed during the implementation? that's why it didn't work with 'legacy' stored value?

You are right, in that specific case the following code didn't worked correctly.

const pinnedFieldsState = storage.get(PINNED_FIELDS_KEY) || {};
storage.set(PINNED_FIELDS_KEY, Object.assign(pinnedFieldsState, { [dataViewId]: newFields }))

Since implementation was changed, pinnedFieldsState here is a string and we cannot assign any properties to string, it was always setting the same string. I have already added validation. Now, even if you change stored value manually, pinned fields anyway will be stored as a new object entry.

@dimaanj dimaanj requested a review from kertal December 21, 2021 14:41
@dimaanj
Copy link
Contributor Author

dimaanj commented Dec 22, 2021

@elasticmachine merge upstream

1 similar comment
@kertal
Copy link
Member

kertal commented Dec 22, 2021

@elasticmachine merge upstream

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.

LGTM, tested again, works as expected, even with invalid data in localStorage, great work 👍

Now you're Discover Claus this year, distributing this nice 🎁
Honored to be your reindeer!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default Firefox Tests / InfraOps App Metrics UI Home page with metrics present group nodes by custom field
  • [job] [logs] OSS Misc Functional Tests / Saved Objects Management saved objects management with hidden types Delete modal should not delete the hidden objects when performing the operation

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 357 360 +3

Async chunks

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

id before after diff
discover 321.2KB 330.6KB +9.4KB

Page load bundle

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

id before after diff
discover 51.3KB 51.5KB +163.0B
Unknown metric groups

async chunk count

id before after diff
discover 6 8 +2

ESLint disabled line counts

id before after diff
discover 33 34 +1

Total ESLint disabled count

id before after diff
discover 35 36 +1

History

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

cc @dmitriynj

@dimaanj dimaanj merged commit 58dac20 into elastic:main Dec 22, 2021
Discover automation moved this from PRs to Done Dec 22, 2021
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 22, 2021
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
* [Discover] add initial table improvements

* [Discover] add pinned fields implementation

* [Discover] simplify the code, fix functional test, fix pagination

* [Discover] fix mobile view

* [Discover] fix unit tests

* [Discover] fix interaction with local storage

* [Discover] apply suggestions

* [Discover] change field name

* [Discover] do not show pinned fields on single doc view page

* [Discover] apply suggestions

* [Discover] adjust typings

* [Discover] update by comments

* [Discover] fix search text

* [Discover] add validation for pinned fields

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
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, Graph) v8.1.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Discover] Improve doc viewer table performance
8 participants