[App Search] Curation: Add query management UX & organic documents list #94488

cee-chen · 2021-03-11T20:45:43Z

Summary

The PR expands the single curation view and adds query management UI/UX & the organic documents section:

Screencaps

Loading states:

QA

Organic results
- Confirm the Result list looks/behaves as expected from other instances of documents in the app
Active query switching
- Confirm that switching values works / changes the organic results returned
- Confirm that deleting the currently active query from the manage queries modal causes the active query select to fall back to the first valid query option
Manage queries modal
- Confirm that adding, deleting, or updating a curation query correctly updates the queries select options
- (Optional) Check/refresh the standalone UI to confirm changes reflect there as well
Expected error testing:
- Attempt to add the same query row twice (e.g. test followed by test again). Confirm you see the following error:
- Attempt to add a query that already exists within the a different curation set (if in the national parks demo: try mountains or hiking). Confirm you see the following error:

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-03-11T20:52:21Z

...e_search/public/applications/app_search/components/curations/curation/curation_logic.test.ts

@@ -60,7 +62,7 @@ describe('CurationLogic', () => {

  describe('actions', () => {
    describe('onCurationLoad', () => {
-      it('should set curation state & dataLoading to false', () => {
+      it('should set curation state & all loading states to false', () => {


A quick note on this - various Curation sections (namely: queries, promoted documents, hidden documents, and organic documents) will each have their own loading state. This is for the sake of UI/UX; so we're still showing responsive reactions but not blanking out the entire page every time we make a single XHR call.

The queries & organic documents loading state images are shown in the PR loading description

The promoted/hidden loading state will use the new LoadingOverlay component: [App Search] DataPanel & LoadingOverlay component tweaks #94251 (comment)

cee-chen · 2021-03-11T21:06:51Z

Actually, after some thought, I'm going to update this PR to include the organic results list as well as the query management UI. It fits the QA portion of this PR since it's affected by the Active Query switcher, and code wise it doesn't add too much more context/complexity (especially if I leave out the promote/demote actions for now).

byronhulcher · 2021-03-12T00:28:47Z

Not sure if this is officially ready for review yet but this is looking good so far.

cee-chen · 2021-03-12T04:30:17Z

Sorry for the wait Byron, I got distracted and walked away before hitting submit on some comments 🤦‍♀️ I'm currently second-guessing myself somewhat on some minor UI tweaks I made to the OrganicDocuments section (primarily the title and the loading UI I ended up using there), but I'll run those by Davey on Monday and address it in a separate commit or PR if so. Thanks for your patience!

cee-chen

OK, this PR has been updated with the Organic Documents section - QA should be slightly more exciting now! Screencaps and QA steps have been summarily updated.

cee-chen · 2021-03-11T23:57:31Z

...rch/public/applications/app_search/components/curations/curation/results/curation_result.tsx

+export const CurationResult: React.FC<Props> = ({ result, actions }) => {
+  const {
+    isMetaEngine,
+    engine: { schema },
+  } = useValues(EngineLogic);


This <CurationResult> component is essentially a super light wrapper around <Result> that auto-populates the isMetaEngine and schemaForTypeHighlights props so that I don't have to do it multiple times for every section (i.e., Organic Documents, Hidden Documents, & Promoted Documents).

It also adds an EuiSpacer after each result as well. Fairly light, but since it's the same props & styling for 3 different sections, I thought it was worth pulling out to its own component.

cee-chen · 2021-03-11T23:58:03Z

...rch/public/applications/app_search/components/curations/curation/results/curation_result.tsx

+    <>
+      <Result
+        result={result}
+        actions={actions}


For reference, result actions were added in #94378

cee-chen · 2021-03-11T23:58:54Z

...public/applications/app_search/components/curations/curation/documents/organic_documents.tsx

+          <CurationResult
+            result={document}
+            key={document.id.raw}
+            actions={[]} // TODO: Next Curation PR


We'll add the actual promote/hide actions & logic in the next PR.

cee-chen · 2021-03-12T00:11:10Z

...k/plugins/enterprise_search/public/applications/app_search/components/curations/constants.ts

+  'xpack.enterpriseSearch.appSearch.engine.curations.resultActionsDescription',
+  { defaultMessage: 'Promote results by clicking the star, hide them by clicking the eye.' }
+);


To help with UX, I'm reusing this copy in the Add Result flyout as well as the Organic Documents sub-description, hence this being in constants.ts.

Also more context: this copy is from the standalone UI. I do wonder if we could make it maybe more descriptive/helpful? But I'm not exactly sure how - Chris will probably have ideas when he does a final copy pass for us

cee-chen · 2021-03-12T05:43:11Z

Actually sorry, I noticed a UI bug while recording my screen capture - when deleting the currently active query, the organic documents screen doesn't show a loading state even though it should (because it's switching currently active queries). I'm going to push up some changes that involves cleaning up updateCuration to update queries from state instead of taking it as an arg (which will be more consistent w/ how the other 3 params being passed in the API call are treated as well).

EDIT: Bug should be fixed now, and I think the logic code is a bit cleaner as well! Success all around (hopefully, if y'all agree):

- updating the active query - adding queriesLoading and organicDocumentsLoading state - clean up / refactor updateCurations slightly - instead of taking a {queries} param, we should opt for a separate updateQueries action and storing a queries state - this allows us to more granularly hook into activeQuery when queries is updated without the current activeQuery and correct for that

- used for switching the currently active query

- used for editing/adding/removing the queries a curation manages - primarily a light wrapper around the existing reusable CurationQueries component (also used in the create new curation view)

+ update breadcrumb to pull from queries instead of curation.queries, mostly for consistency w/ other usages & slightly faster responsiveness when updating queries

byronhulcher

This looks really good, I have some questions but don't consider them blockers.

byronhulcher · 2021-03-15T15:19:26Z

...ublic/applications/app_search/components/curations/curation/queries/manage_queries_modal.tsx

+            <CurationQueries
+              queries={queries}
+              submitButtonText={i18n.translate('xpack.enterpriseSearch.actions.save', {
+                defaultMessage: 'Save',


I like the idea of these generic translations for common actions, should they be moved up to a shared constants file? Trying to think about the "discoverability" of this key for a future dev.

100%!! It's a tech debt item I'm hoping to do this week (I swear, I know I've been saying this for a few weeks now lol. This week or next week for sure though)

byronhulcher · 2021-03-15T15:21:40Z

.../applications/app_search/components/curations/curation/queries/manage_queries_modal.test.tsx

+
+    beforeEach(() => {
+      wrapper = shallow(<ManageQueriesModal />);
+      wrapper.find(EuiButton).simulate('click');


Is it possible to mock the values of useState? I can see how this can act as a replacement for an explicit test on for the presence of an onClick for the button (see my comment above), but I think its better to write explicit tests for them (this calls the state setter, when state is X we render some specific html). This matches more closely to how we test our components with separate logic files that don't contain their own state management.

It's possible to mock useState, but it's kind of a pain. The setup looks like this:

jest.mock('react', () => ({ ...(jest.requireActual('react') as object), useState: jest.fn(), })); import { useState } from 'react'; it('some test', () => { const setter = jest.fn(), (useState as jest.Mock).returnValueOnce(['mock state', setter]); });

I don't strongly object to it, but there are a couple of minor issues I have with the concept:

Once mocked, it's not really possible to have useState work as expected in other tests - the setters won't actually set state as expected. This is actually distinctly unlike our logic files: we can set default state for the logic files, but we can still test that state works as expected & changes based on actions. I'm hesitant to implement this for that reason; it makes it impossible to write non-mocked state tests which I do think is important to test.

I also don't think view / React component tests necessarily need to be written the same as logic or more standard unit tests. They're kind of a different breed of "unit" tests that aren't as straightforward and cover a weird blend of UI/UX and logic. It might be worth codifying those some day for sure.

.../applications/app_search/components/curations/curation/queries/manage_queries_modal.test.tsx

byronhulcher · 2021-03-15T17:32:06Z

.../enterprise_search/public/applications/app_search/components/curations/curation/curation.tsx

+          <ActiveQuerySelect />
+        </EuiFlexItem>
+        <EuiFlexItem grow={false}>
+          <ManageQueriesModal />


The pop-out modal design of this combined with the ActiveQuerySelect above seems strange to me tbh. Messing around with this in the UI (both here and in feels hard to grok how curations -> queries -> curated documents related to each other. In my head I'd expect the list of queries to be the main content of this page, and then each query linking to a subview to manage it. I'm not sure I agree with the organization for this view to have the selector/detail view as the main content, and the list be in a modal. I recognize this is similar to how our current UX in ent-search works, and I'm happy the selector got moved out of the modal, but maybe its worth removing the modal entirely.

Rather than re-organize this whole view though, I'd consider wrapping this custom component out for an EuiComboBox, inline on the page, above the query editor. I think that would better re-enforce "here are the queries you've selected, now you can select one below to edit it's details on this page. If we're worried about friction of deleting a query, we could add a confirm dialogue as we've done elsewhere for deletions. As a bonus, this has a smaller surface area for coverage!

Hmm, I think I'm struggling to picture this UI/UX with the ComboBox - I can't quite visualize how you select the active query in combination with the combobox. So it's a dropdown and then there's a 2nd dropdown for the active query? I guess it's hard for me to picture how that might look next to each other.

I do agree though it does look like the combobox could replace the rows UI/UX + getting rid of an extra modal is always nice! I think @daveyholler might be making another pass/polish round on the Curation page at some point, definitely worth pitching this to him.

In my head I'd expect the list of queries to be the main content of this page, and then each query linking to a subview to manage it

I just want to be clear on this though, the main action/purpose of this page is promoting & hiding documents for the curation, not managing the queries. The list of promoted / hidden documents applies to all queries in the array, those do not change if you change the active query. Changing the active query just lets you see different organic results for that query (although you can manually search for those anyway). The primary functionality of a curation is definitely promoting/pinning documents to the top of the search for specific queries (to my mind, maybe Davey or Casey sees it differently).

From Davey re: Combobox usage

Sweet. I like the idea on the surface. I’ll play around with it and see how it feels. Definitely a solid idea for the polish pass

🎉

byronhulcher · 2021-03-15T17:33:56Z

.../enterprise_search/public/applications/app_search/components/curations/curation/curation.tsx

@@ -38,20 +40,27 @@ export const Curation: React.FC<Props> = ({ curationsBreadcrumb }) => {

  return (
    <>
-      <SetPageChrome trail={[...curationsBreadcrumb, curation.queries.join(', ')]} />
+      <SetPageChrome trail={[...curationsBreadcrumb, queries.join(', ')]} />
      <EuiPageHeader
        pageTitle={MANAGE_CURATION_TITLE}


I think its worth considering adding the queries to this title

As a subtitle, not in the page title, just to clarify? Let me check on this with @daveyholler for sure. For context, here's the standalone UI:

Here's Kibana:

I was hoping the Kibana breadcrumbs + the active query switcher would provide enough context that I didn't need to repeat the queries in a subtitle, but definitely fine with adding it if Davey thinks it's needed

I was originally going to suggest adding the query to the title here as well, but after giving it some thought I agree that the active query selector and breadcrumbs provide enough context without making us have to worry about how to handle long tail queries as an <h1>

Thanks Davey! FWIW, I'd be a +1 to revisiting during a polish pass or if we decide we've lost context after coming back to it.

kibanamachine · 2021-03-15T22:09:56Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1294	1301	+7

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.1MB	2.1MB	+6.5KB

History

💚 Build #113116 succeeded 8e9ab01
💔 Build #112929 failed 87e3b8f
💚 Build #112910 succeeded a811054e720ca3ab9d8fb5d2df055f1ea05cc7c9
💔 Build #112906 failed 5aad7b0d17caec219bc1e2d33c1230dcdf0e0ed3
💚 Build #112867 succeeded 9faa9afda6f983dc800c4a033cbd814c63f9f1c9

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

cee-chen · 2021-03-15T22:38:44Z

Merging in for now, but please feel free to continue any conversations

…st (elastic#94488) * Update CurationLogic to handle queries management - updating the active query - adding queriesLoading and organicDocumentsLoading state - clean up / refactor updateCurations slightly - instead of taking a {queries} param, we should opt for a separate updateQueries action and storing a queries state - this allows us to more granularly hook into activeQuery when queries is updated without the current activeQuery and correct for that * Add ActiveQuerySelect component - used for switching the currently active query * Add ManageQueriesModal component - used for editing/adding/removing the queries a curation manages - primarily a light wrapper around the existing reusable CurationQueries component (also used in the create new curation view) * Add OrganicDocuments and CurationResult components * Update Curation view with new components + update breadcrumb to pull from queries instead of curation.queries, mostly for consistency w/ other usages & slightly faster responsiveness when updating queries * Fix unnecessary import * Meeting feedback: organic documents title copy tweak * PR feedback - test assertion

kibanamachine · 2021-03-15T22:42:25Z

💚 Backport successful

✅ 7.x / #94651

Successful backport PRs will be merged automatically after passing CI.

…st (#94488) (#94651) * Update CurationLogic to handle queries management - updating the active query - adding queriesLoading and organicDocumentsLoading state - clean up / refactor updateCurations slightly - instead of taking a {queries} param, we should opt for a separate updateQueries action and storing a queries state - this allows us to more granularly hook into activeQuery when queries is updated without the current activeQuery and correct for that * Add ActiveQuerySelect component - used for switching the currently active query * Add ManageQueriesModal component - used for editing/adding/removing the queries a curation manages - primarily a light wrapper around the existing reusable CurationQueries component (also used in the create new curation view) * Add OrganicDocuments and CurationResult components * Update Curation view with new components + update breadcrumb to pull from queries instead of curation.queries, mostly for consistency w/ other usages & slightly faster responsiveness when updating queries * Fix unnecessary import * Meeting feedback: organic documents title copy tweak * PR feedback - test assertion Co-authored-by: Constance <[email protected]>

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 Mar 11, 2021

cee-chen requested a review from a team March 11, 2021 20:45

cee-chen commented Mar 11, 2021

View reviewed changes

cee-chen force-pushed the curation-2 branch from 9faa9af to 5aad7b0 Compare March 11, 2021 23:08

cee-chen changed the title ~~[App Search] Curation: Add query management components~~ [App Search] Curation: Add query management UX & organic documents list Mar 11, 2021

cee-chen force-pushed the curation-2 branch from 5aad7b0 to a811054 Compare March 11, 2021 23:49

cee-chen commented Mar 12, 2021

View reviewed changes

cee-chen force-pushed the curation-2 branch from a811054 to a324251 Compare March 12, 2021 05:53

cee-chen added 5 commits March 11, 2021 21:56

Add ActiveQuerySelect component

21a4829

- used for switching the currently active query

Add ManageQueriesModal component

85c94e6

- used for editing/adding/removing the queries a curation manages - primarily a light wrapper around the existing reusable CurationQueries component (also used in the create new curation view)

Add OrganicDocuments and CurationResult components

89df558

Update Curation view with new components

87e3b8f

+ update breadcrumb to pull from queries instead of curation.queries, mostly for consistency w/ other usages & slightly faster responsiveness when updating queries

cee-chen force-pushed the curation-2 branch from a324251 to 87e3b8f Compare March 12, 2021 06:04

byronhulcher self-requested a review March 15, 2021 15:36

cee-chen added 2 commits March 15, 2021 08:52

Fix unnecessary import

cd7ef24

Merge branch 'master' into curation-2

8e9ab01

byronhulcher approved these changes Mar 15, 2021

View reviewed changes

cee-chen added 2 commits March 15, 2021 12:46

Meeting feedback: organic documents title copy tweak

2c484e0

PR feedback - test assertion

824a918

cee-chen merged commit 5396d27 into elastic:master Mar 15, 2021

cee-chen deleted the curation-2 branch March 15, 2021 22:39

kibanamachine mentioned this pull request Mar 15, 2021

[7.x] [App Search] Curation: Add query management UX & organic documents list (#94488) #94651

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Curation: Add query management UX & organic documents list #94488

[App Search] Curation: Add query management UX & organic documents list #94488

cee-chen commented Mar 11, 2021 •

edited

Loading

cee-chen Mar 11, 2021 •

edited

Loading

cee-chen commented Mar 11, 2021 •

edited

Loading

byronhulcher commented Mar 12, 2021

cee-chen commented Mar 12, 2021

cee-chen left a comment

cee-chen Mar 11, 2021

cee-chen Mar 11, 2021

cee-chen Mar 11, 2021

cee-chen Mar 12, 2021

cee-chen commented Mar 12, 2021 •

edited

Loading

byronhulcher left a comment

byronhulcher Mar 15, 2021

cee-chen Mar 15, 2021

byronhulcher Mar 15, 2021

cee-chen Mar 15, 2021 •

edited

Loading

byronhulcher Mar 15, 2021

cee-chen Mar 15, 2021 •

edited

Loading

cee-chen Mar 15, 2021

byronhulcher Mar 15, 2021

cee-chen Mar 15, 2021

daveyholler Mar 15, 2021

cee-chen Mar 15, 2021

kibanamachine commented Mar 15, 2021

cee-chen commented Mar 15, 2021

kibanamachine commented Mar 15, 2021

[App Search] Curation: Add query management UX & organic documents list #94488

[App Search] Curation: Add query management UX & organic documents list #94488

Conversation

cee-chen commented Mar 11, 2021 • edited Loading

Summary

Screencaps

QA

Checklist

cee-chen Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen commented Mar 11, 2021 • edited Loading

byronhulcher commented Mar 12, 2021

cee-chen commented Mar 12, 2021

cee-chen 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

cee-chen commented Mar 12, 2021 • edited Loading

byronhulcher 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

cee-chen Mar 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Mar 15, 2021 • edited Loading

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

kibanamachine commented Mar 15, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

cee-chen commented Mar 15, 2021

kibanamachine commented Mar 15, 2021

💚 Backport successful

cee-chen commented Mar 11, 2021 •

edited

Loading

cee-chen Mar 11, 2021 •

edited

Loading

cee-chen commented Mar 11, 2021 •

edited

Loading

cee-chen commented Mar 12, 2021 •

edited

Loading

cee-chen Mar 15, 2021 •

edited

Loading

cee-chen Mar 15, 2021 •

edited

Loading