[App Search] Migrate expanded rows for meta engines table in Engines Overview #96251

byronhulcher · 2021-04-05T19:26:54Z

Summary

Migrates the expanded rows from the Meta Engines table containing information about the source engines and their conflicts for each meta engine. This required the creation of a new MetaEnginesTable component, with its own logic file responsible for retrieving the source engines for each meta engine from the API when the user requests it. We also have some logic to inform the user about schema conflicts, and which source engines are responsible for them.

On mobile this is usable but having a responsive table inside a responsive table is a little rough. This could be a good candidate for a design pass later.

Screenshots

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser) (looks a little rough though 😅 )
This was checked for cross-browser compatibility

…ngines

byronhulcher · 2021-04-05T21:49:02Z

...c/applications/app_search/components/engines/components/meta_engines_table_expanded_row.scss

+  @include euiBreakpoint('l', 'xl') {
+    .euiTableRowCell {
+      border-top: none;
+    }
+
+    .euiTitle {
+      display: none;
+    }
+  }


Is there a better way to accomplish this?

After thinking about this and how the expanded row works I don't think so, currently. One possible option would be to not nest an EuiBasicTable within the expanded row and instead try to directly use cell components, e.g. <EuiTableRowCell> - but then you lose some of the automatic mobile UX that comes with the basic table.

I think this is fine for now, especially since after testing the expanded row behavior in a screenreader, it appears that EUI's implementation of expanded rows has a known issue where screen readers cannot dive into the expanded row, so it's not really possible to test this accessibility-wise anyway.

It might be interesting to revisit it someday to try and compare implementations, but I'm definitely good with a nested table for now (which I learned today is semantically valid! 🤯)

...ic/applications/app_search/components/engines/components/meta_engines_table_expanded_row.tsx

cee-chen

General folder architecture request: Can we move all table-related components/logic/styles to its own directory? I'm fine with either:

engines/
|_ components/
  |_tables/
    |- engines_table.tsx
    |- meta_engines_table_expansion.tsx
    |- meta_engines_table_logic.tsx
    |- meta_engines_table.tsx
    |_ // etc.

or skipping components/ and just giving it its own tables/ dir

engines/
|_ tables/
  |- engines_table.tsx
  |- meta_engines_table_expansion.tsx
  |- meta_engines_table_logic.tsx
  |- meta_engines_table.tsx
  |_ // etc.

LMK what you think!

On another shared architecture note - I'd love it if the shared columns between the 2 tables got pulled out to a columns.tsx or shared_columns.tsx file rather than only living in engines_table.tsx - I think that will make it easier to find/see when exploring the component from a directory POV.

cee-chen

First pass of feedback - I have a few more subjective or higher level thoughts/suggestions, but need to stop for today - will come back to this tomorrow!

x-pack/plugins/enterprise_search/public/applications/app_search/components/engines/utils.ts

...erprise_search/public/applications/app_search/components/engines/meta_engines_table_logic.ts

...se_search/public/applications/app_search/components/engines/meta_engines_table_logic.test.ts

...s/enterprise_search/public/applications/app_search/components/engines/meta_engines_table.tsx

...ications/app_search/components/engines/components/meta_engines_table_name_column_content.tsx

...ic/applications/app_search/components/engines/components/meta_engines_table_expanded_row.tsx

...c/applications/app_search/components/engines/components/meta_engines_table_expanded_row.scss

...ic/applications/app_search/components/engines/components/meta_engines_table_expanded_row.tsx

byronhulcher · 2021-04-08T12:13:12Z

General folder architecture request: [...]

If so, can we DRY this out to a shared types.ts file that lives under the new tables/ folder I proposed in my earlier comment?

Yes to both of these.

Thanks for reviewing this so far @constancecchen, I'm going to be pushing updates in response to this review, but will anticipate another round from you today/next week.

…Content

byronhulcher · 2021-04-12T15:54:35Z

Hey @constancecchen I've pushed some updates based on your feedback. Let me know what else I can do to improve this. Even if we decide to have you take over the PR I still think I could do another round today.

...cations/app_search/components/engines/components/tables/meta_engines_table_expanded_row.scss

...ications/app_search/components/engines/components/tables/meta_engines_table_expanded_row.tsx

...s/app_search/components/engines/components/tables/meta_engines_table_name_column_content.tsx

...ise_search/public/applications/app_search/components/engines/components/tables/utils.test.ts

...earch/public/applications/app_search/components/engines/components/tables/shared_columns.tsx

cee-chen

Here's an optional round of feedback that contain a few higher-level / more complex questions/requests. If those would take too long to do, definitely feel free to say you'd rather defer and I can grab it in a follow-up PR or take over the PR. Alternatively if you're strongly not a fan, I'd love to hear your thoughts

...h/public/applications/app_search/components/engines/components/tables/meta_engines_table.tsx

...s/app_search/components/engines/components/tables/meta_engines_table_name_column_content.tsx

..._search/components/engines/components/tables/meta_engines_table_name_column_content.test.tsx

...lic/applications/app_search/components/engines/components/tables/meta_engines_table.test.tsx

byronhulcher · 2021-04-12T20:13:27Z

Hey @constancecchen another round your way. I hit some of the suggestions you made in the third round of feedback, but passed on the ones involving abstraction. I've given you access to this remote repo, its up to you if you'd rather take over the branch or do a follow-up PR, I'm happy either way as long as this is in 7.13 😎

byronhulcher · 2021-04-12T21:10:12Z

@elasticmachine merge upstream

byronhulcher · 2021-04-13T11:57:24Z

@elasticmachine merge upstream

byronhulcher · 2021-04-13T16:06:14Z

@elasticmachine merge upstream

+ update to account for 2 previous DRY commits (e.g. deleteEngine mock)

kibanamachine · 2021-04-14T01:15:35Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1332	1348	+16

Async chunks

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

id	before	after	diff
`enterpriseSearch`	2.0MB	2.0MB	+11.0KB

History

💔 Build #119291 failed d78c067
💛 Build #119172 was flaky 821f132
💔 Build #118970 failed 024b1f2
💚 Build #119009 succeeded a24ee83
💚 Build #118847 succeeded 39146c5

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

cc @byronhulcher

kibanamachine · 2021-04-14T01:18:23Z

💔 Backport failed

Status	Branch	Result
❌	7.14	The branch "7.14" is invalid or doesn't exist

To backport manually run:
node scripts/backport --pr 96251

…ax_primary_shard_size * 'master' of github.com:elastic/kibana: (99 commits) added missing optional chain for bracket notation (elastic#96939) [Discover][DocViewer] Fix toggle columns from doc viewer table tab (elastic#95748) [TSVB] Fix per-request caching of index patterns (elastic#97043) [Datatable] Fix filter cell flakiness (elastic#96934) Unskip heatmap suite and fixes flakiness (elastic#96941) [Fleet] Improve performance of data stream API (elastic#97058) [ML] Data Frame Analytics: remove beta badge (elastic#96977) [App Search] Migrate expanded rows for meta engines table in Engines Overview (elastic#96251) Instances latency distribution chart tooltips and axis fixes (elastic#95577) [Monitoring] Using primary average shard size (elastic#96177) [Workplace Search] Hide Kibana chrome on 3rd party connector redirects (elastic#97028) ## [Security Solution] Fixes `Exit full screen` and `Copy to cliboard` styling issues (elastic#96676) Index pattern field editor - Add warning on name or type change (elastic#95528) [App Search] Add small engine breadcrumb utility helper (elastic#96917) Copy esArchiver commands from ./reassign.ts to fix tests (elastic#97012) [Security Solution][Detections] Updates MITRE Tactics, Techniques, and Subtechniques for 7.13 (elastic#97011) Index patterns server - throw correct error on field caps 404 (elastic#95879) Use `EuiThemeProvider` in lists plugin tests and stories (elastic#96129) [npm] upgrade caniuse database (elastic#97002) chore(NA): moving @kbn/apm-utils into bazel (elastic#96227) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/serialization/policy_serialization.test.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/schema.ts

…to-metrics-tab * 'master' of github.com:elastic/kibana: (61 commits) [Usage collection] Usage counters (elastic#96696) UI actions readme (elastic#96925) [TSVB] Enable brush for visualizations created with no index patterns (elastic#96727) [Data telemetry] Add Async Search to the tests (elastic#96693) added missing optional chain for bracket notation (elastic#96939) [Discover][DocViewer] Fix toggle columns from doc viewer table tab (elastic#95748) [TSVB] Fix per-request caching of index patterns (elastic#97043) [Datatable] Fix filter cell flakiness (elastic#96934) Unskip heatmap suite and fixes flakiness (elastic#96941) [Fleet] Improve performance of data stream API (elastic#97058) [ML] Data Frame Analytics: remove beta badge (elastic#96977) [App Search] Migrate expanded rows for meta engines table in Engines Overview (elastic#96251) Instances latency distribution chart tooltips and axis fixes (elastic#95577) [Monitoring] Using primary average shard size (elastic#96177) [Workplace Search] Hide Kibana chrome on 3rd party connector redirects (elastic#97028) ## [Security Solution] Fixes `Exit full screen` and `Copy to cliboard` styling issues (elastic#96676) Index pattern field editor - Add warning on name or type change (elastic#95528) [App Search] Add small engine breadcrumb utility helper (elastic#96917) Copy esArchiver commands from ./reassign.ts to fix tests (elastic#97012) [Security Solution][Detections] Updates MITRE Tactics, Techniques, and Subtechniques for 7.13 (elastic#97011) ...

cee-chen · 2021-04-14T14:48:59Z

Will take care of the manual backport for this: #97123

…Overview (#96251) (#97123) * Pull out columns to be re-used for MetaEnginesTable * Add route to get source engines for meta engines * New MetaEnginesTableLogic * New MetaEnginesTable component * Remove isMeta prop from EnginesTable * Swap EnginesTable with MetaEnginesTable in EnginesOverview for meta engines * Missing test for MetaEnginesTableNameColumnContent * Created new /app_search/components/engines/components/tables directory * Moving columns to shared_columns.tsx file * Updates to MetaEnginesTableExpandedRow and MetaEnginesTableNameColumnContent * Fixes to EnginesTable, MetaEnginesTable, MetaEnginesTableLogic * Remove flatten import * Fix i18n * PR Feedback * DRY out shared engine link helpers * DRY out shared ACTIONS_COLUMN * Tests: DRY out shared columns/props tests + update to account for 2 previous DRY commits (e.g. deleteEngine mock) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance Chen <constance.chen.3@gmail.com> Co-authored-by: Byron Hulcher <byronhulcher@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…Overview (elastic#96251) * Pull out columns to be re-used for MetaEnginesTable * Add route to get source engines for meta engines * New MetaEnginesTableLogic * New MetaEnginesTable component * Remove isMeta prop from EnginesTable * Swap EnginesTable with MetaEnginesTable in EnginesOverview for meta engines * Missing test for MetaEnginesTableNameColumnContent * Created new /app_search/components/engines/components/tables directory * Moving columns to shared_columns.tsx file * Updates to MetaEnginesTableExpandedRow and MetaEnginesTableNameColumnContent * Fixes to EnginesTable, MetaEnginesTable, MetaEnginesTableLogic * Remove flatten import * Fix i18n * PR Feedback * DRY out shared engine link helpers * DRY out shared ACTIONS_COLUMN * Tests: DRY out shared columns/props tests + update to account for 2 previous DRY commits (e.g. deleteEngine mock) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

byronhulcher added 7 commits April 5, 2021 13:58

Pull out columns to be re-used for MetaEnginesTable

c28c2b9

Add route to get source engines for meta engines

85e8857

New MetaEnginesTableLogic

34c8798

New MetaEnginesTable component

0cf74f5

Remove isMeta prop from EnginesTable

30dc408

Swap EnginesTable with MetaEnginesTable in EnginesOverview for meta e…

fc882da

…ngines

Missing test for MetaEnginesTableNameColumnContent

3770f9e

byronhulcher commented Apr 5, 2021

View reviewed changes

byronhulcher marked this pull request as ready for review April 5, 2021 21:55

byronhulcher requested a review from a team April 5, 2021 21:55

byronhulcher added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels Apr 5, 2021

byronhulcher self-assigned this Apr 5, 2021

cee-chen requested changes Apr 7, 2021

View reviewed changes

cee-chen reviewed Apr 7, 2021

View reviewed changes

byronhulcher added 7 commits April 8, 2021 13:27

Created new /app_search/components/engines/components/tables directory

6982ce0

Moving columns to shared_columns.tsx file

aabd29a

Updates to MetaEnginesTableExpandedRow and MetaEnginesTableNameColumn…

a364bf8

…Content

Fixes to EnginesTable, MetaEnginesTable, MetaEnginesTableLogic

dbfcc82

Merge remote-tracking branch 'origin/master' into meta-engines-table

391331e

Merge remote-tracking branch 'origin/master' into meta-engines-table

0bac01f

Remove flatten import

a29c45a

byronhulcher requested a review from cee-chen April 12, 2021 15:52

Fix i18n

39146c5

cee-chen reviewed Apr 12, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into meta-engines-table

b9aba42

PR Feedback

024b1f2

byronhulcher requested a review from cee-chen April 12, 2021 20:09

Merge branch 'master' into meta-engines-table

a24ee83

Merge branch 'master' into meta-engines-table

821f132

kibanamachine and others added 4 commits April 13, 2021 12:06

Merge branch 'master' into meta-engines-table

d78c067

DRY out shared engine link helpers

abeda3c

DRY out shared ACTIONS_COLUMN

5f30950

Tests: DRY out shared columns/props tests

6fcf91c

+ update to account for 2 previous DRY commits (e.g. deleteEngine mock)

cee-chen approved these changes Apr 13, 2021

View reviewed changes

cee-chen enabled auto-merge (squash) April 13, 2021 23:12

cee-chen merged commit 71672c4 into elastic:master Apr 14, 2021

cee-chen added v7.13.0 and removed v7.14.0 labels Apr 14, 2021

cee-chen mentioned this pull request Apr 14, 2021

[7.x] [App Search] Migrate expanded rows for meta engines table in Engines Overview (#96251) #97123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Migrate expanded rows for meta engines table in Engines Overview #96251

[App Search] Migrate expanded rows for meta engines table in Engines Overview #96251

byronhulcher commented Apr 5, 2021 •

edited

Loading

byronhulcher Apr 5, 2021

cee-chen Apr 9, 2021

cee-chen left a comment •

edited

Loading

cee-chen left a comment

byronhulcher commented Apr 8, 2021

byronhulcher commented Apr 12, 2021

cee-chen left a comment •

edited

Loading

byronhulcher commented Apr 12, 2021

byronhulcher commented Apr 12, 2021

byronhulcher commented Apr 13, 2021

byronhulcher commented Apr 13, 2021

kibanamachine commented Apr 14, 2021

kibanamachine commented Apr 14, 2021

cee-chen commented Apr 14, 2021

[App Search] Migrate expanded rows for meta engines table in Engines Overview #96251

[App Search] Migrate expanded rows for meta engines table in Engines Overview #96251

Conversation

byronhulcher commented Apr 5, 2021 • edited Loading

Summary

Screenshots

Checklist

byronhulcher Apr 5, 2021

Choose a reason for hiding this comment

cee-chen Apr 9, 2021

Choose a reason for hiding this comment

cee-chen left a comment • edited Loading

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

byronhulcher commented Apr 8, 2021

byronhulcher commented Apr 12, 2021

cee-chen left a comment • edited Loading

Choose a reason for hiding this comment

byronhulcher commented Apr 12, 2021

byronhulcher commented Apr 12, 2021

byronhulcher commented Apr 13, 2021

byronhulcher commented Apr 13, 2021

kibanamachine commented Apr 14, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Apr 14, 2021

💔 Backport failed

cee-chen commented Apr 14, 2021

byronhulcher commented Apr 5, 2021 •

edited

Loading

cee-chen left a comment •

edited

Loading

cee-chen left a comment •

edited

Loading