[UnifiedFieldList] Migrate field list components from Lens to UnifiedFieldList #142758

jughosta · 2022-10-05T14:40:50Z

Summary

This PR migrates components/logic from Lens to UnifiedFieldList. There should be no visual changes at this point.

Extract Lens components to UnifiedFieldList plugin
Create a new hook useExistingFieldsFetcher for fetching fields existence data. A separate hook useExistingFieldsReader will be used for reading the fetched data in children components (instead of reading from Lens redux state).
Create useGroupedFields hook for grouping a fields list into sections
Integrate these components back into Lens page
Adjust after [Lens] Rename datasources #142553
Adjust after [Lens]Adds selected field accordion to the fields list #143175
Refactor text-based languages sidebar view to use the same common components
Add more tests and update docs

As our next step we integrate them into Discover page.

Notes for testing:
Please test field list on Lens page. It should behave as before for regular/ad-hoc data views and for text-based queries. Fields existence data was removed from Lens redux store and the layers panel now uses useExistingFieldsReader hook to get the existence data - please check that ui controls work as before. Please also check that the performance of the sidebar with a lot of fields was not affected.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
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

…ocs'

…ce info

…d-list-sections # Conflicts: # x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx

…-fix'

…d-list-sections # Conflicts: # src/plugins/unified_field_list/public/components/field_list/field_list_grouped.scss # src/plugins/unified_field_list/public/components/field_list/fields_accordion.tsx # src/plugins/unified_field_list/public/components/field_list/no_fields_callout.test.tsx # src/plugins/unified_field_list/public/components/field_list/no_fields_callout.tsx # x-pack/plugins/lens/public/data_views_service/loader.test.ts # x-pack/plugins/lens/public/data_views_service/loader.ts # x-pack/plugins/lens/public/datasources/form_based/datapanel.test.tsx # x-pack/plugins/lens/public/datasources/form_based/datapanel.tsx # x-pack/plugins/lens/public/datasources/form_based/dimension_panel/dimension_panel.test.tsx # x-pack/plugins/lens/public/datasources/form_based/dimension_panel/field_input.test.tsx # x-pack/plugins/lens/public/datasources/form_based/dimension_panel/field_select.tsx # x-pack/plugins/lens/public/datasources/form_based/dimension_panel/reference_editor.tsx # x-pack/plugins/lens/public/datasources/form_based/field_list.scss # x-pack/plugins/lens/public/datasources/form_based/field_list.tsx # x-pack/plugins/lens/public/datasources/form_based/fields_accordion.test.tsx # x-pack/plugins/lens/public/datasources/form_based/fields_accordion.tsx # x-pack/plugins/lens/public/datasources/form_based/no_fields_callout.test.tsx # x-pack/plugins/lens/public/datasources/form_based/no_fields_callout.tsx # x-pack/plugins/lens/public/datasources/form_based/operations/definitions/index.ts # x-pack/plugins/lens/public/datasources/form_based/operations/definitions/terms/field_inputs.tsx # x-pack/plugins/lens/public/indexpattern_datasource/field_list.scss # x-pack/plugins/lens/public/indexpattern_datasource/fields_accordion.tsx # x-pack/plugins/lens/public/indexpattern_datasource/no_fields_callout.test.tsx # x-pack/plugins/lens/public/indexpattern_datasource/no_fields_callout.tsx

jughosta · 2022-10-25T08:23:34Z

@dej611 Updated. Thanks for reviewing the PR!

davismcphee

Finished my code review and it looks great 👍 The documentation is nice to have and lots of test coverage is always good. I left a few questions and comments, and I still have to do some more local testing to do, but it LGTM so far.

src/plugins/unified_field_list/README.md

src/plugins/unified_field_list/public/components/field_list/field_list_grouped.tsx

src/plugins/unified_field_list/public/components/field_list/index.tsx

src/plugins/unified_field_list/public/hooks/use_existing_fields.ts

src/plugins/unified_field_list/public/hooks/use_grouped_fields.ts

davismcphee · 2022-10-26T19:20:38Z

src/plugins/unified_field_list/public/hooks/use_grouped_fields.ts

+        isAffectedByGlobalFilter: false,
+        isAffectedByTimeFilter: true,
+        // Show details on timeout but not failure
+        // hideDetails: fieldsExistenceInfoUnavailable && !existenceFetchTimeout, // TODO: is this check still necessary?


Do we know if this is necessary now or do we possibly need input from the Lens team?

The question is how to catch timeouts of loadFieldExisting after it was moved from Lens server API to public of unifiedFieldList plugin in #139453. It might be not relevant anymore.

cc @dimaanj

I also think that this is no longer necessary since dataViewsService getFieldsForIndexPattern doesn't support a dedicated timeout error state, right @mattkime ?

davismcphee

I got a chance to finish my local testing today and if there are issues, I can't find them at least 👍 Everything worked great and I didn't encounter anything that seemed like a bug to me.

The only thing I noticed that seemed a little strange was that once I added a field to my vis, the plus button in its popover became disabled and the tooltip description didn't seem correct:

This isn't a blocker to me though (and it might have already worked that way in Lens), but I wonder if doing something like switching the plus button to a minus when the field is selected would be more helpful to a user instead:

Other than that, this LGTM as soon as we get the lastFetchRequestedAtTimestamp concerns sorted out.

…d-list-sections # Conflicts: # x-pack/plugins/lens/public/datasources/text_based/fields_accordion.tsx

jughosta · 2022-10-28T11:20:39Z

@elasticmachine merge upstream

jughosta · 2022-10-28T12:26:02Z

@davismcphee I think the behaviour of "+" button becoming disabled in the popover for a selected field is correct. It's the same on the main branch and the tooltip is the same.

…r will have their own tooltips.

davismcphee

Thanks for the updates! I did a final round of testing and code review, and it LGTM 🎉 It seems like there might still be a couple outstanding questions in the comments before we hit the button, but on my end this seems ready to merge. I'm excited to see this implemented in Discover too 🙂

I think the behaviour of "+" button becoming disabled in the popover for a selected field is correct. It's the same on the main branch and the tooltip is the same.

Thanks for checking that for me, and it's good to know the behaviour is the same as before. I probably should have written "I think disabling the button and keeping the tooltip isn't a great UX" instead, but that's not something for this PR to address anyway.

src/plugins/unified_field_list/public/components/field_list/index.tsx

jughosta · 2022-11-01T14:00:28Z

Hi @elastic/kibana-vis-editors,
Can I ask for another review for this refactoring?

stratoula

This works fine Julia! I can't find any regression on functionality or performance. Thanx for working on that :) LGTM

kibana-ci · 2022-11-02T09:01:08Z

💚 Build Succeeded

Buildkite Build
Commit: 871d361

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	895	885	-10
`unifiedFieldList`	32	49	+17
total			+7

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
`lens`	583	580	-3
`unifiedFieldList`	117	187	+70
total			+67

Async chunks

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

id	before	after	diff
`lens`	1.3MB	1.3MB	-13.5KB
`unifiedFieldList`	20.4KB	33.3KB	+12.9KB
total			-608.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`unifiedFieldList`	2	4	+2

Page load bundle

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

id	before	after	diff
`unifiedFieldList`	15.8KB	21.0KB	+5.1KB

Unknown metric groups

API count

id	before	after	diff
`lens`	676	673	-3
`unifiedFieldList`	122	192	+70
total			+67

async chunk count

id	before	after	diff
`unifiedFieldList`	2	4	+2

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	57	63	+6
`lens`	20	19	-1
`osquery`	103	108	+5
`securitySolution`	439	445	+6
`unifiedFieldList`	9	13	+4
total			+22

References to deprecated APIs

id	before	after	diff
`lens`	31	16	-15
`unifiedFieldList`	30	40	+10
total			-5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	65	71	+6
`lens`	23	22	-1
`osquery`	104	110	+6
`securitySolution`	516	522	+6
`unifiedFieldList`	9	13	+4
total			+23

History

💚 Build #84346 succeeded 3ec55d9
💚 Build #83835 succeeded 124c82b
💚 Build #83523 succeeded 8c75546
💚 Build #83093 succeeded cdbceeb
💚 Build #82709 succeeded 27f99f9
💚 Build #82382 succeeded 1acc00d

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

cc @jughosta

dej611

Tested again in Safari and found no new bugs from main 👍

…to Discover (#144412) Closes #135678 ## Summary This PR continues the work started in #142758 to bring field list grouping from Lens into Discover. - [x] Integrate new components and hooks into Discover page - [x] Refactor fields grouping logic - [x] Render Popular fields under a new separate section - [x] Remove "Hide empty fields" switch - [x] Adjust filtering logic - [x] Refactor fields existence logic in Discover - [x] Add "Unmapped fields" section - [x] Highlight the matching term when searching for a field - [x] Show field icons when in SQL mode - [x] Add tooltips to field list section headings - [x] Add tests, clean up <img width="340" alt="Screenshot 2022-11-15 at 15 39 27" src="https://user-images.githubusercontent.com/1415710/201947349-726ffc3a-a17f-411b-be92-81d97879765a.png"> For testing on Discover page: Please check different use cases and toggling Advanced Settings: - regular vs ad-hoc data views - data views with and without a time field - data views with unmapped and empty fields - data views with a lot of fields - data views with some fields being filtered out via data view configuration - updating query, filters, and time range - regular and SQL mode - searching by a field name in the sidebar - applying a field filter in the sidebar - adding, editing, and removing a field - Field Statistics table when some columns are selected or no columns are selected - multifields in the field popover should work as before (icon should change from "+" to "x" when subfield is selected as a column) - `discover:searchOnPageLoad` should not show fields if turned off - `discover:searchFieldsFromSource` should show multifields right in the fields list if enabled - `discover:enableSql` should show Selected and Available fields only when enabled - `discover:showLegacyFieldTopValues` should show old (green) field stats in its popover - `doc_table:legacy` On Lens page: - scroll position should reset when data view is switched or when searching by a field name - regular and SQL mode ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [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 renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) Co-authored-by: Michael Marcialis <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Stratoula Kalafateli <[email protected]>

jughosta added 2 commits October 5, 2022 14:16

[UnifiedFieldList] Extract FieldsAccordion component from Lens

7b33d6b

[UnifiedFieldList] Extract FieldList component from Lens

61975d8

jughosta added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.6.0 labels Oct 5, 2022

jughosta self-assigned this Oct 5, 2022

kibanamachine and others added 23 commits October 5, 2022 15:28

[CI] Auto-commit changed files from 'node scripts/build_plugin_list_d…

8a5c3ab

…ocs'

[UnifiedFieldList] Rename component

21ba7b4

[UnifiedFieldList] Start extracting logic for fetching fields existen…

8cb999d

…ce info

[UnifiedFieldList] Start extracting logic for fetching fields existen…

ae1b988

…ce info

[UnifiedFieldList] Fix special and runtime fields

87f8c1b

[UnifiedFieldList] Start extracting logic for fetching fields existen…

8d9a819

…ce info

Merge remote-tracking branch 'upstream/main' into 135678-unified-fiel…

ff199ea

…d-list-sections # Conflicts: # x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

b972d67

…-fix'

[UnifiedFieldList] Make API stricter

a84a26a

[UnifiedFieldList] Make sure that key is specified for items

46bd6e6

[UnifiedFieldList] Fetch data for all active data views

a755602

[UnifiedFieldList] Refactor some other occurances

f0ce67d

[UnifiedFieldList] Update more tests

6035e90

[UnifiedFieldList] Fix some checks

2db2300

[UnifiedFieldList] Some progress on updating tests

78f40c8

[UnifiedFieldList] Update more tests

5135262

[UnifiedFieldList] Skip redundant request's results

94f7cd3

[UnifiedFieldList] Update more tests

63775a3

[UnifiedFieldList] Improve tests

b5759d7

[UnifiedFieldList] Improve tests

4d6df85

[UnifiedFieldList] Improve tests

76f3138

[UnifiedFieldList] Move grouping into a customizable hook

5a004d5

MichaelMarcialis approved these changes Oct 25, 2022

View reviewed changes

kertal added the Feature:UnifiedFieldList The unified field list component used by Lens & Discover label Oct 25, 2022

Merge branch 'main' into 135678-unified-field-list-sections

27f99f9

jughosta requested a review from a team as a code owner October 26, 2022 07:02

jughosta mentioned this pull request Oct 26, 2022

[Discover][UnifiedFieldList] Integrate unified field list sections into Discover #144045

Closed

13 tasks

davismcphee reviewed Oct 26, 2022

View reviewed changes

jughosta added 2 commits October 27, 2022 11:34

Merge branch 'main' into 135678-unified-field-list-sections

a27f79c

[UnifiedFieldList] Address PR comments

cdbceeb

davismcphee reviewed Oct 27, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into 135678-unified-fiel…

0de6254

…d-list-sections # Conflicts: # x-pack/plugins/lens/public/datasources/text_based/fields_accordion.tsx

kibanamachine and others added 2 commits October 28, 2022 07:20

Merge branch 'main' into 135678-unified-field-list-sections

3b5aaea

[UnifiedFieldList] Support a custom data-test-subj

a66e7b5

jughosta added 2 commits October 28, 2022 15:06

[UnifiedFieldList] Fix concurrency handling logic

8e1892f

Merge branch 'main' into 135678-unified-field-list-sections

8c75546

jughosta requested a review from dej611 October 28, 2022 13:56

jughosta added 2 commits October 31, 2022 10:06

Merge branch 'main' into 135678-unified-field-list-sections

59a3590

[UnifiedFieldList] Remove a generic tooltip message. Lens and Discove…

124c82b

…r will have their own tooltips.

davismcphee approved these changes Oct 31, 2022

View reviewed changes

src/plugins/unified_field_list/public/components/field_list/index.tsx Show resolved Hide resolved

Merge branch 'main' into 135678-unified-field-list-sections

3ec55d9

Merge branch 'main' into 135678-unified-field-list-sections

871d361

stratoula approved these changes Nov 2, 2022

View reviewed changes

dej611 approved these changes Nov 2, 2022

View reviewed changes

jughosta merged commit ca1c58d into elastic:main Nov 2, 2022

jughosta mentioned this pull request Nov 2, 2022

[Discover][UnifiedFieldList] Integrate unified field list sections into Discover #144412

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[UnifiedFieldList] Migrate field list components from Lens to UnifiedFieldList #142758

[UnifiedFieldList] Migrate field list components from Lens to UnifiedFieldList #142758

jughosta commented Oct 5, 2022 •

edited

Loading

jughosta commented Oct 25, 2022

davismcphee left a comment

davismcphee Oct 26, 2022

jughosta Oct 27, 2022

kertal Oct 28, 2022

davismcphee left a comment

jughosta commented Oct 28, 2022

jughosta commented Oct 28, 2022

davismcphee left a comment

jughosta commented Nov 1, 2022

stratoula left a comment

kibana-ci commented Nov 2, 2022

API count

async chunk count

ESLint disabled in files

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

dej611 left a comment

[UnifiedFieldList] Migrate field list components from Lens to UnifiedFieldList #142758

[UnifiedFieldList] Migrate field list components from Lens to UnifiedFieldList #142758

Conversation

jughosta commented Oct 5, 2022 • edited Loading

Summary

Checklist

jughosta commented Oct 25, 2022

davismcphee left a comment

Choose a reason for hiding this comment

davismcphee Oct 26, 2022

Choose a reason for hiding this comment

jughosta Oct 27, 2022

Choose a reason for hiding this comment

kertal Oct 28, 2022

Choose a reason for hiding this comment

davismcphee left a comment

Choose a reason for hiding this comment

jughosta commented Oct 28, 2022

jughosta commented Oct 28, 2022

davismcphee left a comment

Choose a reason for hiding this comment

jughosta commented Nov 1, 2022

stratoula left a comment

Choose a reason for hiding this comment

kibana-ci commented Nov 2, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

async chunk count

ESLint disabled in files

ESLint disabled line counts

References to deprecated APIs

Total ESLint disabled count

History

dej611 left a comment

Choose a reason for hiding this comment

jughosta commented Oct 5, 2022 •

edited

Loading