[App Search] Refactor out a shared MultiInputRows component #96881

cee-chen · 2021-04-12T19:45:12Z

Summary

👋 Jason noted a while back that my CurationQueries component is super similar to a piece of UI in Relevance Tuning. As it turns out, the upcoming Synonyms view will ALSO use an identical component:

As the saying goes, once is chance, twice is coincidence, and three times is a pattern... so let's DRY it out! 🎉 Doing so also has added UI/UX benefit, as the shared component does some cool stuff like:

Disabling the delete row button when it's down to just 1 row
Disabling the add row button if there's already a blank row
Disabling the submit button if there's only 1 row and it's blank

BTW, I strongly recommend following along by commit, and reading the commit messages!

Screencaps

Curations

Relevance Tuning

QA

Go to the Create Curations page and confirm adding/removing queries still works as expected
Go to Curations > select any curation > open the Manage Queries modal and confirm adding/removing queries still works as expected
Go to Revelance Tuning and confirm adding/removing value boosts still works as expected

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)
This was checked for cross-browser compatibility

cee-chen · 2021-04-12T19:53:16Z

...lugins/enterprise_search/public/applications/app_search/components/multi_input_rows/index.ts

@@ -5,4 +5,4 @@
 * 2.0.
 */

-export { CurationQueries } from './curation_queries';
+export { MultiInputRows } from './multi_input_rows';


Just to clarify:

This is indeed essentially just the the CurationQueries component but relabelled, I basically moved the entire folder out to its own top-level component and did a bunch of find&replacing to more generic names.

What's changed is that it allows passing in overrides for all the various text/input/aria labels, and also a onChange functionality was added for the Relevance Tuning use-case

I'm not super in love with the MultiInputRows name lol, it's just what came to mind. Definitely open to ideas if y'all have them

...prise_search/public/applications/app_search/components/multi_input_rows/multi_input_rows.tsx

cee-chen · 2021-04-12T20:21:08Z

@elasticmachine merge upstream

..._search/public/applications/app_search/components/multi_input_rows/multi_input_rows.test.tsx

...prise_search/public/applications/app_search/components/multi_input_rows/multi_input_rows.tsx

..._search/public/applications/app_search/components/multi_input_rows/multi_input_rows_logic.ts

JasonStoltz · 2021-04-13T20:40:07Z

I need to drop off for now, will continue reviewing tomorrow!

- basically the CurationQuery component, but with a generic values var & allows passing in custom text for every string

- for upcoming Relevance Tuning usage

- relevance_tuning_form.test.tsx fix: was getting test errors with mount(), so I switched to shallow()

- more flexible - allows for either an onSubmit or onChange, or even potentially both

- so that we can have multiple instances on the same page - primarily the value boosts use case

- Use Kea's types instead of trying to rewrite my own LogicFile - Add an early return for tests that pass `{}` to values as well for performance

JasonStoltz

This looks so good. I'm going to approve. 2 suggestions to consider before continuing and that's it.

...prise_search/public/applications/app_search/components/multi_input_rows/multi_input_rows.tsx

JasonStoltz · 2021-04-14T18:16:38Z

I really like that this change pulls a lot of the complexity out of the Relevance Tuning logic file and encapsulates it in this component. This is really great.

+ bonus - add a fallback for initially empty components + add a test to check that the logic was mounted correctly

- We don't currently need the extra catch for any live components, and it's confusing

cee-chen · 2021-04-14T20:06:56Z

@elasticmachine merge upstream

cee-chen · 2021-04-14T20:08:28Z

:dead_inside: don't mind me, just over here overloading Kibana's CI instances because I'm a ding dong

…96881) * Add new reusable MultiInputRows component - basically the CurationQuery component, but with a generic values var & allows passing in custom text for every string * Update CurationQueries with MultiInputRows * Update MultiInputRows to support on change behavior - for upcoming Relevance Tuning usage * Update Relevance Tuning value boost form to use new component - relevance_tuning_form.test.tsx fix: was getting test errors with mount(), so I switched to shallow() * Change submitOnChange to onChange fn - more flexible - allows for either an onSubmit or onChange, or even potentially both * Convert MultiInputRowsLogic to keyed Kea logic - so that we can have multiple instances on the same page - primarily the value boosts use case * Update LogicMounter helper & tests to handle keyed logic w/ props * [Misc] LogicMounter helper - fix typing, perf - Use Kea's types instead of trying to rewrite my own LogicFile - Add an early return for tests that pass `{}` to values as well for performance * PR feedback: Change values prop to initialValues + bonus - add a fallback for initially empty components + add a test to check that the logic was mounted correctly * PR feedback: Remove useRef/on mount onChange catch for now - We don't currently need the extra catch for any live components, and it's confusing

kibanamachine · 2021-04-14T22:13:46Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…97193) * Add new reusable MultiInputRows component - basically the CurationQuery component, but with a generic values var & allows passing in custom text for every string * Update CurationQueries with MultiInputRows * Update MultiInputRows to support on change behavior - for upcoming Relevance Tuning usage * Update Relevance Tuning value boost form to use new component - relevance_tuning_form.test.tsx fix: was getting test errors with mount(), so I switched to shallow() * Change submitOnChange to onChange fn - more flexible - allows for either an onSubmit or onChange, or even potentially both * Convert MultiInputRowsLogic to keyed Kea logic - so that we can have multiple instances on the same page - primarily the value boosts use case * Update LogicMounter helper & tests to handle keyed logic w/ props * [Misc] LogicMounter helper - fix typing, perf - Use Kea's types instead of trying to rewrite my own LogicFile - Add an early return for tests that pass `{}` to values as well for performance * PR feedback: Change values prop to initialValues + bonus - add a fallback for initially empty components + add a test to check that the logic was mounted correctly * PR feedback: Remove useRef/on mount onChange catch for now - We don't currently need the extra catch for any live components, and it's confusing Co-authored-by: Constance <constancecchen@users.noreply.github.com>

kibanamachine · 2021-04-19T17:36:42Z

💚 Build Succeeded

Metrics [docs]

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	-1.4KB

History

💚 Build #119766 succeeded 782c91c
💔 Build #119737 failed 3931eb3
💚 Build #119719 succeeded e628d4d
💔 Build #119483 failed 8436c812adc2283505da786e93580de936453ebc
💚 Build #118982 succeeded 7ed0d78fd8c584a6cb6cc2d991dcbf55ece8647f

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

cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 12, 2021

cee-chen requested review from a team and JasonStoltz April 12, 2021 19:45

cee-chen commented Apr 12, 2021

View reviewed changes

...prise_search/public/applications/app_search/components/multi_input_rows/multi_input_rows.tsx Outdated Show resolved Hide resolved

JasonStoltz reviewed Apr 13, 2021

View reviewed changes

..._search/public/applications/app_search/components/multi_input_rows/multi_input_rows.test.tsx Outdated Show resolved Hide resolved

...prise_search/public/applications/app_search/components/multi_input_rows/multi_input_rows.tsx Outdated Show resolved Hide resolved

JasonStoltz reviewed Apr 13, 2021

View reviewed changes

..._search/public/applications/app_search/components/multi_input_rows/multi_input_rows_logic.ts Outdated Show resolved Hide resolved

cee-chen force-pushed the multi-input-rows branch from 7ed0d78 to 8436c81 Compare April 14, 2021 00:54

cee-chen added 6 commits April 14, 2021 10:45

Add new reusable MultiInputRows component

c337530

- basically the CurationQuery component, but with a generic values var & allows passing in custom text for every string

Update CurationQueries with MultiInputRows

89e0076

Update MultiInputRows to support on change behavior

e89b4e0

- for upcoming Relevance Tuning usage

Update Relevance Tuning value boost form to use new component

5977d20

- relevance_tuning_form.test.tsx fix: was getting test errors with mount(), so I switched to shallow()

Change submitOnChange to onChange fn

2c02f16

- more flexible - allows for either an onSubmit or onChange, or even potentially both

Convert MultiInputRowsLogic to keyed Kea logic

85b95a1

- so that we can have multiple instances on the same page - primarily the value boosts use case

cee-chen force-pushed the multi-input-rows branch from 8436c81 to a758b1f Compare April 14, 2021 17:47

cee-chen requested a review from a team April 14, 2021 17:47

cee-chen added 2 commits April 14, 2021 10:58

Update LogicMounter helper & tests to handle keyed logic w/ props

909cf4b

[Misc] LogicMounter helper - fix typing, perf

e628d4d

- Use Kea's types instead of trying to rewrite my own LogicFile - Add an early return for tests that pass `{}` to values as well for performance

cee-chen force-pushed the multi-input-rows branch from a758b1f to e628d4d Compare April 14, 2021 18:00

JasonStoltz approved these changes Apr 14, 2021

View reviewed changes

...prise_search/public/applications/app_search/components/multi_input_rows/multi_input_rows.tsx Outdated Show resolved Hide resolved

...prise_search/public/applications/app_search/components/multi_input_rows/multi_input_rows.tsx Outdated Show resolved Hide resolved

cee-chen added 2 commits April 14, 2021 11:48

PR feedback: Change values prop to initialValues

5d97f84

+ bonus - add a fallback for initially empty components + add a test to check that the logic was mounted correctly

PR feedback: Remove useRef/on mount onChange catch for now

3931eb3

- We don't currently need the extra catch for any live components, and it's confusing

cee-chen enabled auto-merge (squash) April 14, 2021 18:54

Merge branch 'master' into multi-input-rows

782c91c

cee-chen merged commit 2a281c9 into elastic:master Apr 14, 2021

kibanamachine mentioned this pull request Apr 14, 2021

[7.x] [App Search] Refactor out a shared MultiInputRows component (#96881) #97193

Merged

cee-chen deleted the multi-input-rows branch April 19, 2021 15:32

cee-chen mentioned this pull request Jul 8, 2021

[App Search] Relevance Tuning: Fix unsaved changes bug #104951

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Refactor out a shared MultiInputRows component #96881

[App Search] Refactor out a shared MultiInputRows component #96881

cee-chen commented Apr 12, 2021 •

edited

Loading

cee-chen Apr 12, 2021 •

edited

Loading

cee-chen commented Apr 12, 2021

JasonStoltz commented Apr 13, 2021

JasonStoltz left a comment

JasonStoltz commented Apr 14, 2021

cee-chen commented Apr 14, 2021

cee-chen commented Apr 14, 2021

kibanamachine commented Apr 14, 2021

kibanamachine commented Apr 19, 2021

[App Search] Refactor out a shared MultiInputRows component #96881

[App Search] Refactor out a shared MultiInputRows component #96881

Conversation

cee-chen commented Apr 12, 2021 • edited Loading

Summary

Screencaps

Curations

Relevance Tuning

QA

Checklist

cee-chen Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen commented Apr 12, 2021

JasonStoltz commented Apr 13, 2021

JasonStoltz left a comment

Choose a reason for hiding this comment

JasonStoltz commented Apr 14, 2021

cee-chen commented Apr 14, 2021

cee-chen commented Apr 14, 2021

kibanamachine commented Apr 14, 2021

💚 Backport successful

kibanamachine commented Apr 19, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

cee-chen commented Apr 12, 2021 •

edited

Loading

cee-chen Apr 12, 2021 •

edited

Loading