[ES|QL] Lookup index editor - New layout and controls

[sddonne]

Part of #235730

Summary

• New way of inserting rows and columns
• Simplified layout
• Rows height has been adjusted
• We are now displaying 50 rows by default.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios
• Flaky Test Runner was used on any tests changed

[elasticmachine]

Pinging @elastic/kibana-esql (Team:ESQL)

[kibanamachine]

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#9933

[✅] src/platform/test/functional/apps/discover/esql/config.ts: 50/50 tests passed.

see run history

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: 5774bd3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
esql	509	510	+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	389	390	+1

Async chunks

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

id	before	after	diff
cloudSecurityPosture	643.0KB	643.2KB	+212.0B
discover	1.2MB	1.2MB	+212.0B
esql	669.7KB	672.9KB	+3.2KB
esqlDataGrid	150.2KB	150.4KB	+210.0B
securitySolution	11.1MB	11.1MB	+210.0B
slo	987.9KB	988.1KB	+212.0B
total			+4.3KB

Unknown metric groups

API count

id	before	after	diff
@kbn/discover-utils	451	452	+1

ESLint disabled line counts

id	before	after	diff
@kbn/index-editor	4	3	-1

Total ESLint disabled count

id	before	after	diff
@kbn/index-editor	4	3	-1

[davismcphee]

Data Discovery changes LGTM 👍

Also tested a bit locally and left a few comments, but nothing blocking. I haven't been testing the lookup editor much lately though so not sure how much of it existed before this PR.

[davismcphee]

I noticed a couple of cases where content was cut off.

First the header and remove button when uploading a CSV (also seems odd that we can collapse the section IMO):

And truncated cells in the table don't use and ellipsis and are just cut off:

[davismcphee]

I also encountered a couple of behaviours when editing columns that might be unexpected.

The first is that when I change a field type then cancel it, the cancelled selection is preserved when editing again:

change_field_type.mp4

The other is that after changing the field type of a column, the selection disappears when editing again:

change_field_type_2.mp4

[davismcphee]

I tested out the keyboard navigation and it works pretty well overall, but I noticed that once I focused on a select cell, I was stuck there. And when I tried to use the esc key to get out, it closed the whole flyout:

focus_select.mp4

[davismcphee]

And just a minor design nit. Feels like we could remove the margins to the left/right of the table header, and below the table. It just looks a bit misaligned right now imo.

Current:

Without margins:

[stratoula]

Thanks Seb this looks very nice! Some small comments from me

[stratoula]

We could declare them in another file and import them here for a cleaner component (nit)

[stratoula]

This seems weird:

dataView && dataViewColumns ? (
                  <DataGridLazy
                    {...props}
                    dataView={dataView}
                    columns={dataViewColumns}
                    rows={rows}
                    totalHits={totalHits}
                    onOpenIndexInDiscover={props.onOpenIndexInDiscover}
                  />
                ) : (
                <EuiSkeletonText />
              )

Do this instead

dataView ? (
                dataViewColumns ? (
                  <DataGridLazy
                    {...props}
                    dataView={dataView}
                    columns={dataViewColumns}
                    rows={rows}
                    totalHits={totalHits}
                    onOpenIndexInDiscover={props.onOpenIndexInDiscover}
                  />
                ) : null
              ) : (
                <EuiSkeletonText />
              )

[stratoula]

This can be simplified

  const returnFocus = useCallback(() => {
    if (columnIndex === undefined) {
      return true;
    }

    requestAnimationFrame(() => {
      const headerWrapper = findElementBySelectorOrRef(`[${COLUMN_INDEX_PROP}="${columnIndex}"]`);
      headerWrapper?.focus();
    });

    return false;
  }, [columnIndex]);

[stratoula]

Suggested change

	closePopover={() => closePopover()}
	closePopover={closePopover}

[stratoula]

Suggested change

	const openInDiscoverText = i18n.translate('indexEditor.toolbar.openInDiscover', {
	const openInDiscoverTooltip = i18n.translate('indexEditor.toolbar.openInDiscoverTooltip', {

[stratoula]

This Given, when, then comments seem weird tbh, I see we do it in the other tests too but they are unnecessary imho, can we remove them?

[stratoula]

Let's run the flaky runner to be sure that we don't introduce any flakiness

[stratoula]

I have already a lookup index, I add a new column but I don't see the save button

[PhilippeOberti]

LGTM for the @elastic/security-threat-hunting-investigations team