[App Search] Create Curation view/functionality #92560

cee-chen · 2021-02-24T03:37:38Z

Summary

This adds the "Create new curation" view to the curations section. It also essentially adds the "Manage queries" modal that will be used on the individual curation view, as the CurationQueries component is shared between these two views.

QA

Go to http://localhost:5601/xyz/app/enterprise_search/app_search/engines/national-parks-demo/curations
Confirm clicking the "Create new curation" button takes you to the correct page
Confirm the "continue" button is disabled if there no query fields are filled out, and enabled if there are any fields with content
Confirm clicking the "Add query" button adds a new row
Confirm the trash icon button is disabled if there's only 1 row remaining, and enabled once more rows are added
Confirm that attempting to add a query that already has a curation (e.g. mountains) displays an error
Confirm that creating a new curation query works and takes you to a curation page that works on the standalone UI
Confirm that creating a new query with an empty row correctly filters out the blank query

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

- to match createCuration - IMO, this language isn't necessary if we're splitting up Curations and CurationLogic - the context is fairly evident within a smaller and more modular logic file

+ accompanying CurationQueriesLogic & CurationQuery row

cee-chen · 2021-02-24T04:11:06Z

...ns/enterprise_search/public/applications/app_search/components/curations/components/index.ts

+ * 2.0.
+ */
+
+export { CurationQueries } from './curation_queries';


👋 Just want to highlight this commit (f6dc151) and note where I've made some significant differences from the Standalone UI.

<CurationQueries /> and <CurationQuery /> are almost completely different from ent-search, and require significantly less logic & branching. With @daveyholler's approval and guidance, I've simplified the UX of the curation queries component significantly which results in a massive code cleanup.

The component is used in two places: The Create New view and the Manage Queries modal

Create curation page

Before

After

Changes

Removed unnecessary multi-toggle - Davey agreed it does not add any significant confusion to have curation queries default to multiple

Allow removing rows - this was functionality present in the modal but hidden for the create view, which feels confusing/inconsistent. I strongly think the ability to control/remove rows should always be present for maximum user agency

Change EuiFieldSearch to EuiFieldText: this was done for semantic reasons as an input type="search" is simply not the correct field to be using in this scenario - since nothing is actually being searched. This loses us the magnifying glass icon and the ability to clear the input, but IMO the loss there is minimal

Manage queries modal

Before

After

Changes

As you can see, the main UX issue here complicating the 'before' view is the concept of "selecting" the currently active query. It forces the concept of an "editable" vs a "non-editable/selectable" query row.

Davey suggested (and I strongly agree) that it makes far more sense to create a select/dropdown on the main curation page itself for switching/selecting the current active query, and have this modal purely in charge of editing/deleting queries. This vastly simplifies & cleans up the component to be able to :

Always use editable form components, instead of adding any branching clickable interactivity

Removes the need for a "cancel" button, instead allowing us to simplify to always having the same remove row icon/functionality

Remove singleton "Save" buttons - instead, we can just save once, at the end of the form

Thoughts?

Let me know what y'all think! I think there's potentially some room here as well to use Byron's InlineEditableTable component that he made for Crawler, but I don't want to migrate that over right now, and I think this is a fairly straightforward compromise that is hopefully easy to review (let me know if not) and involves wayyy less code than what's currently in ent-search. I'm definitely open to investigating InlineEditableTable later post-MVP.

I think it's a great improvement.

cee-chen · 2021-02-24T04:21:18Z

...ations/app_search/components/curations/components/curation_queries/curation_queries_logic.ts

+  editQuery(index: number, newQueryValue: string): { index: number; newQueryValue: string };
+}
+
+export const CurationQueriesLogic = kea<


👋 Just want to highlight this logic file as well - this actually doesn't strictly need to be a logic file and it's very lightweight, but I thought it was kinda fun/helped separate concerns to make it one. This was my original proof of concept with just basic React state:

export const CurationQueries: React.FC = (props) => { // TODO: logic file rather than state?? const [queries, setQueries] = useState(props.queries || ['']); const addQuery = () => { setQueries([...queries, '']); }; const deleteQuery = (index: number) => { const newArray = [...queries]; newArray.splice(index, 1); setQueries(newArray); }; const changeQuery = (index: number, newQueryValue: string) => { const newArray = [...queries]; newArray[index] = newQueryValue; setQueries(newArray); }; const hasEmptyQueries = queries.indexOf('') >= 0; const hasOnlyOneQuery = queries.length <= 1;

Would be interested in hearing thoughts on what you do/don't prefer 🤷 I do think having it as logic makes it way easier to unit test.

Another thing I played around with was keying this logic file with an id, but it actually made testing kind of a pain (since you have to store that keyed logic file reference in a var and can't just refer back to the plain imported CurationQueriesLogic), and I also decided I didn't really need it since the component fully unmounts/mounts between page visits/modal closures.

I like this quite a bit constance. +1 for separation of concerns and easier unit testing.

JasonStoltz · 2021-02-25T14:51:08Z

Quick testing note ... after I create a curation, I don't see the button to launch a modal as seen in your gif

JasonStoltz · 2021-02-25T14:52:20Z

Also, amazing work on the simplification. Seriously commendable, I really like this new experience.

cee-chen · 2021-02-25T16:15:54Z

Oh yeah sorry, the modal is not in this PR, just in my local. It's in the gifs to show to Davey & get his approval for its future use/UI/UX, and give context as to why this piece of logic is out in its own component vs only living on the creation page, and also why we need the ability to pass it a pre-defined set of queries. Hope that makes sense!

BTW, I also realized after QAing your recent relevance tuning PR that it's super similar to the value boosts rows, which I really like - more consistent UI/UX = happier users, just IMO! 😁

JasonStoltz · 2021-02-25T16:29:25Z

@constancecchen I was waiting to say anything until I dug into your code a bit more, but I like your UI better, it's cleaner and does stuff like disables the trash cans, etc. It'd be cool to retrofit some of that to Relevance Tuning, or even create a shared component. Not sure if there's other places we'd use that or not.

daveyholler · 2021-02-25T16:34:09Z

This is great work @constancecchen. I think this is a great example of improving the experience while porting stuff over. This is gonna make future improvements so much easier.

JasonStoltz

This is fantastic. I have some questions and comments, but nothing big. Let me know once you've responded and I'll approve.

JasonStoltz · 2021-02-25T14:57:55Z

...prise_search/public/applications/app_search/components/curations/views/curation_creation.tsx

+
+  return (
+    <>
+      <EuiPageHeader>


I saw in the latest EUI update email that EuiPageHeader now has props for various header elements, pageTitle, description, rightSideItems, etc.

This will be good to use as it's A) Less code and B) more consistently enforcing the same size / style on every page.

I think it would be good to update all of our pages to use these options. It's up to you whether you update this here, now, or if we wait to do a separate PR and do them all at once.

I did use these in my PR as I needed all three of those properties: https://github.com/elastic/kibana/pull/92644/files#diff-fe1fd9bb49e0518f49db2e6b5888c1f628a8314fb7c3d17353a35d07ab64d128R64-R73

Yo this is awesome!!!!!!!! I totally missed this in the EUI newsletter somehow, I LOVE it. I'll make the change in this PR now but we should most definitely do a product-wide update (I probably have some time during my SDH week next week).

JasonStoltz · 2021-02-25T16:58:33Z

...pplications/app_search/components/curations/components/curation_queries/curation_queries.tsx

+        size="s"
+        iconType="plusInCircle"
+        onClick={addQuery}
+        isDisabled={hasEmptyQueries}


Did you consider adding a test for this being disabled? I don't think it's required for code coverage, but it might be good to document this behavior via a test.

Great call, thanks!

JasonStoltz · 2021-02-25T17:14:02Z

...pplications/app_search/components/curations/components/curation_queries/curation_queries.tsx

+import { CurationQueriesLogic } from './curation_queries_logic';
+import { CurationQuery } from './curation_query';
+import { filterEmptyQueries } from './utils';
+import './curation_queries.scss';


You include this in 2 different components.

Doh, awesome catch

JasonStoltz · 2021-02-25T17:15:46Z

...ations/app_search/components/curations/components/curation_queries/curation_queries_logic.ts

+  editQuery(index: number, newQueryValue: string): { index: number; newQueryValue: string };
+}
+
+export const CurationQueriesLogic = kea<


I like this quite a bit constance. +1 for separation of concerns and easier unit testing.

...s/app_search/components/curations/components/curation_queries/curation_queries_logic.test.ts

JasonStoltz · 2021-02-25T17:19:26Z

.../applications/app_search/components/curations/components/curation_queries/curation_query.tsx

+        color="danger"
+        onClick={onDelete}
+        isDisabled={disableDelete}
+        aria-label={i18n.translate('xpack.enterpriseSearch.actions.delete', {


This is a pretty generic name, is there a risk of collision with other places where this is used?

xpack.enterpriseSearch.actions.delete

This was intentional, at some point (maybe next week if I have time?) I'd like to DRY out these really small actions/button labels like "Edit", "Delete", "Save", "Cancel", "Continue" etc. strings so we just reuse the same const/string and don't make localizers translate them repeatedly.

...rch/public/applications/app_search/components/curations/components/curation_queries/utils.ts

JasonStoltz · 2021-02-25T17:23:17Z

...ns/enterprise_search/public/applications/app_search/components/curations/components/index.ts

+ * 2.0.
+ */
+
+export { CurationQueries } from './curation_queries';


I think it's a great improvement.

JasonStoltz

LGTM!

kibanamachine · 2021-02-25T22:06:55Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1265	1281	+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`	1.9MB	1.9MB	+8.3KB

History

💔 Build #109545 failed 038fc4d
💚 Build #108974 succeeded 732ee56
💔 Build #108973 failed 0ce2645a3e22edd68046173870683ac9a001efd7
💔 Build #108971 failed 241f31b51bca1667230b31b1acb3a7004d33ae59

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

cc @JasonStoltz

* Add server route and logic listener * [Misc] Remove 'Set' from 'deleteCurationSet' - to match createCuration - IMO, this language isn't necessary if we're splitting up Curations and CurationLogic - the context is fairly evident within a smaller and more modular logic file * Add CurationQueries component + accompanying CurationQueriesLogic & CurationQuery row * Add CurationCreation view

kibanamachine · 2021-02-25T22:09:50Z

💚 Backport successful

✅ 7.x / #92923

Successful backport PRs will be merged automatically after passing CI.

* Add server route and logic listener * [Misc] Remove 'Set' from 'deleteCurationSet' - to match createCuration - IMO, this language isn't necessary if we're splitting up Curations and CurationLogic - the context is fairly evident within a smaller and more modular logic file * Add CurationQueries component + accompanying CurationQueriesLogic & CurationQuery row * Add CurationCreation view Co-authored-by: Constance <constancecchen@users.noreply.github.com>

…bana into task-manager/docs-monitoring * 'task-manager/docs-monitoring' of github.com:gmmorris/kibana: (40 commits) [Security Solution][Case][Bug] Improve case logging (elastic#91924) [Alerts][Doc] Added README documentation for alerts plugin status and framework health checks configuration options. (elastic#92761) Add warning for EQL and Threshold rules if exception list contains value list items (elastic#92914) [Security Solution][Case] Fix subcases bugs on detections and case view (elastic#91836) [APM] Always allow access to Profiling via URL (elastic#92889) [Vega] Allow image loading without CORS policy by changing the default to crossOrigin=null (elastic#91991) skip flaky suite (elastic#92114) [APM] Fix for default fields in correlations view (elastic#91868) (elastic#92090) chore(NA): bump bazelisk to v1.7.5 (elastic#92905) [Maps] fix selecting EMS basemap does not populate input (elastic#92711) API docs (elastic#92827) [kbn/test] add import/export support to KbnClient (elastic#92526) Test fix management scripted field filter functional test and unskip it (elastic#92756) [App Search] Create Curation view/functionality (elastic#92560) [Reporting/Discover] include the document's entire set of fields (elastic#92730) [Fleet] Add new index to fleet for artifacts being served out of fleet-server (elastic#92860) [Alerts][Doc] Added README documentation for API key invalidation configuration options. (elastic#92757) [Discover][docs] Add search for relevance (elastic#90611) [Alerts][Docs] Extended README.md and the user docs with the licensing information. (elastic#92564) [7.12][Telemetry] Security telemetry allowlist fix. (elastic#92850) ...

cee-chen added 2 commits February 23, 2021 18:57

Add server route and logic listener

132b6e9

[Misc] Remove 'Set' from 'deleteCurationSet'

209810a

- to match createCuration - IMO, this language isn't necessary if we're splitting up Curations and CurationLogic - the context is fairly evident within a smaller and more modular logic file

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 Feb 24, 2021

cee-chen requested review from JasonStoltz, byronhulcher and a team February 24, 2021 03:37

Add CurationQueries component

f6dc151

+ accompanying CurationQueriesLogic & CurationQuery row

cee-chen force-pushed the curations-creation branch 2 times, most recently from 241f31b to 0ce2645 Compare February 24, 2021 04:00

cee-chen commented Feb 24, 2021

View reviewed changes

Add CurationCreation view

732ee56

cee-chen force-pushed the curations-creation branch from 0ce2645 to 732ee56 Compare February 24, 2021 04:18

cee-chen commented Feb 24, 2021

View reviewed changes

JasonStoltz self-assigned this Feb 25, 2021

JasonStoltz requested changes Feb 25, 2021

View reviewed changes

cee-chen added 4 commits February 25, 2021 10:13

Merge branch 'master' into curations-creation

3e5b31b

[PR feedback] Add test

814152f

[PR feedback] remove extra scss import

3f70ef8

[PR feedback] Use fancy new EuiPageHeader props

038fc4d

cee-chen requested a review from JasonStoltz February 25, 2021 18:43

JasonStoltz approved these changes Feb 25, 2021

View reviewed changes

Fix test after EuiPageHeader change

0f5a530

cee-chen enabled auto-merge (squash) February 25, 2021 20:10

cee-chen merged commit 0198607 into elastic:master Feb 25, 2021

kibanamachine mentioned this pull request Feb 25, 2021

[7.x] [App Search] Create Curation view/functionality (#92560) #92923

Merged

cee-chen deleted the curations-creation branch February 25, 2021 22:12

cee-chen mentioned this pull request Mar 9, 2021

[Enterprise Search] Upgrade Kea to 2.3, update LogicMounter helper w/ props support #94232

Merged

2 tasks

cee-chen mentioned this pull request Apr 12, 2021

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

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Create Curation view/functionality #92560

[App Search] Create Curation view/functionality #92560

cee-chen commented Feb 24, 2021 •

edited

Loading

cee-chen Feb 24, 2021 •

edited

Loading

JasonStoltz Feb 25, 2021

cee-chen Feb 24, 2021 •

edited

Loading

JasonStoltz Feb 25, 2021

JasonStoltz commented Feb 25, 2021

JasonStoltz commented Feb 25, 2021

cee-chen commented Feb 25, 2021

JasonStoltz commented Feb 25, 2021

daveyholler commented Feb 25, 2021

JasonStoltz left a comment

JasonStoltz Feb 25, 2021

cee-chen Feb 25, 2021

cee-chen Feb 25, 2021

JasonStoltz Feb 25, 2021

cee-chen Feb 25, 2021

cee-chen Feb 25, 2021

JasonStoltz Feb 25, 2021

cee-chen Feb 25, 2021

cee-chen Feb 25, 2021

JasonStoltz Feb 25, 2021

JasonStoltz Feb 25, 2021

cee-chen Feb 25, 2021

JasonStoltz Feb 25, 2021

JasonStoltz left a comment

kibanamachine commented Feb 25, 2021

kibanamachine commented Feb 25, 2021

[App Search] Create Curation view/functionality #92560

[App Search] Create Curation view/functionality #92560

Conversation

cee-chen commented Feb 24, 2021 • edited Loading

Summary

QA

Checklist

cee-chen Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Create curation page

Before

After

Changes

Manage queries modal

Before

After

Changes

Thoughts?

Choose a reason for hiding this comment

cee-chen Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz commented Feb 25, 2021

JasonStoltz commented Feb 25, 2021

cee-chen commented Feb 25, 2021

JasonStoltz commented Feb 25, 2021

daveyholler commented Feb 25, 2021

JasonStoltz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 25, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Feb 25, 2021

💚 Backport successful

cee-chen commented Feb 24, 2021 •

edited

Loading

cee-chen Feb 24, 2021 •

edited

Loading

cee-chen Feb 24, 2021 •

edited

Loading