[App Search] Suggestions Callouts for Engine, Analytics, and Manual Curation Detail by byronhulcher · Pull Request #113864 · elastic/kibana

byronhulcher · 2021-10-04T22:45:20Z

Summary

Implements two new components:

SuggestedCurationsCallout, included on our Engine and Analytics pages
SuggestedDocumentsCallout, included on our Curation Detail page for manual curations

These are powered by an underlying SuggestionsCallout component that utilizes localstorage (through our useLocalStorage hook) to track whether the callout has been dismissed or not. The callout uses location.pathname to identify whether it has been dismissed already or not. This assumes that only one SuggestionsCallout is present per view/unique location.pathname.

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)
x] 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

byronhulcher · 2021-10-04T22:48:52Z

...ations/app_search/components/engine_overview/components/suggested_curations_callout.test.tsx

This is weird, but it seems more readable than {...MOCK_VALUES, {...MOCK_VALUES.curation, curation: etc etc, and I think its better than just writing a whole new object, because this highlights specifically whats changed vs the defaults. But it is kinda weird (I wish lodash.set didn't mutate the source object)

See comment above about lodash fp

Thanks for the tip! I did this in a2a3a8a

…en dismissed

byronhulcher · 2021-10-05T00:29:15Z

...earch/public/applications/app_search/components/curations/components/suggestions_callout.tsx

+              iconType="eyeClosed"
+              size="s"
+              onClick={() => {
+                setLastDismissedTimestamp(new Date().toISOString());


An ISO timestamp is of form 2021-10-04T22:53:20.588Z. This matches the style of the timestamp we get back from the API. We've used both ISO timestamps and unix timestamps interchangeably in our codebase so far.

byronhulcher · 2021-10-05T02:10:37Z

This has conflicts but should still be reviewed, they will be resolved once #113885 is merged

JasonStoltz

I'm good with this, just a couple of optional suggestions to consider. Let me know what you think.

JasonStoltz · 2021-10-05T14:19:41Z

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

+import { ENGINE_CURATION_SUGGESTION_PATH } from '../../../routes';
+import { generateEnginePath } from '../../engine';
+
+import { SuggestionsCallout } from '../components/suggestions_callout';


Nice, I like how these share a base callout component.

JasonStoltz · 2021-10-05T14:20:46Z

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

+    curation: { suggestion, queries },
+  } = useValues(CurationLogic);
+
+  if (typeof suggestion === 'undefined' || suggestion.status !== 'pending') {


Just curious why you check for undefined rather than falsy.

Would be nice to have statuses in an enum or at constants.

Just curious why you check for undefined rather than falsy.

To be honest, I default to checking specifically for undefined so I don't have to remember whether false/0/""/{} etc are a valid value for the variable or not.

Would be nice to have statuses in an enum or at constants.

This field uses a union to enforce specific values

status: 'pending' | 'applied' | 'automated' | 'rejected' | 'disabled';

VSCode and tsc will complain if you use a value here that's not one of these strings. And I can still hover over stuggestion.status and it lists all the valid strings. In fact its kind of more useful than enums because for an SuggestionStatus enum it would just say status: SuggestionStatus.

When I used enums on Crawler there was some pushback against that and towards using unions. I dunno which is better tbh. For what its worth I think using a union still enforces a single "source of truth" for valid values, and I think its unlikely we'd change these valid values (that would be a breaking API change) so I'm not super worried about having to search/replace this. Anyway tsc would let us know which values we forgot to update.

OK, maybe adding the strings to a constants file at least. We can deal with that in a different PR if we want.

JasonStoltz · 2021-10-05T14:23:20Z

...c/applications/app_search/components/curations/curation/suggested_documents_callout.test.tsx

+
+  it('is empty when curation status is not pending', () => {
+    const values = cloneDeep(MOCK_VALUES); // set mutates so we cloneDeep first
+    set(values, 'curation.suggestion.status', 'applied');


Nice, using set makes these tests more concise, I like it.

I think you could use lodash/fp which is immutable...

import { set } from 'lodash/fp'; set('curation.suggestion.status', 'applied', values)

That would avoid the cloneDeep

Thanks for the tip, I did this in a2a3a8a

JasonStoltz · 2021-10-05T14:39:35Z

x-pack/plugins/enterprise_search/public/applications/app_search/components/engine/types.ts


+interface SearchRelevanceSuggestions {
+  count: number;
+  curation: {


Do we have a type for curation already that we could use here?

So this is more like a summary/details type than the CurationSuggestion type use get back from our endpoints. These are aggregated details coming from an internal endpoint. We do this for meta engines and crawler as well to track similar top-level engine level information we want to be aware of in the whole Engine UX. I tried to clarify that with new names in e0ed6a4

JasonStoltz · 2021-10-05T14:40:02Z

...ations/app_search/components/engine_overview/components/suggested_curations_callout.test.tsx

See comment above about lodash fp

tylersmalley · 2021-10-06T01:12:53Z

@elasticmachine merge upstream

tylersmalley · 2021-10-06T01:40:27Z

@elasticmachine merge upstream

kibanamachine · 2021-10-06T03:44:58Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1517	1520	+3

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.4MB	1.4MB	+2.8KB

History

💔 Build #158271 failed 91240ea
💔 Build #158237 failed 5840b55
💔 Build #158062 failed e0ed6a4
💔 Build #157856 failed 6b38c07
💔 Build #157830 failed d9a33bb1b50132ab34227e1ae0fb6babf9e1ef42

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

…uration Detail (elastic#113864)

kibanamachine · 2021-10-06T04:01:27Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…uration Detail (#113864) (#114051) Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>

byronhulcher commented Oct 4, 2021

View reviewed changes

byronhulcher force-pushed the suggestions-callout branch 2 times, most recently from ecf9a61 to 7b1a5bd Compare October 4, 2021 23:13

byronhulcher added 7 commits October 4, 2021 20:21

New SuggestionsCallout component

209d30f

New SuggestedDocumentsCallout component

c98ccea

Add SuggestedDocumentsCallout to ManualCurations

2e502a7

New SuggestedCurationsCallout component

55426db

Add SuggestedCurationsCallout to EngineOverviewMetrics

3d556f2

Add SuggestedCurationsCallout to Analytics

08299a2

Use pathname for key to determine whether a SuggestionsCallout has be…

9771822

…en dismissed

byronhulcher force-pushed the suggestions-callout branch from 7b1a5bd to 9771822 Compare October 5, 2021 00:24

byronhulcher commented Oct 5, 2021

View reviewed changes

byronhulcher marked this pull request as ready for review October 5, 2021 00:30

byronhulcher requested review from a team and JasonStoltz October 5, 2021 00:30

byronhulcher changed the title ~~Suggestions Callouts for Engine, Analytics, and Manual Curation Detail~~ [App Search] Suggestions Callouts for Engine, Analytics, and Manual Curation Detail Oct 5, 2021

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

byronhulcher force-pushed the suggestions-callout branch from 56a2234 to d9a33bb Compare October 5, 2021 01:08

Clean-up test mock values

6b38c07

byronhulcher force-pushed the suggestions-callout branch from d9a33bb to 6b38c07 Compare October 5, 2021 03:25

JasonStoltz requested changes Oct 5, 2021

View reviewed changes

byronhulcher added 2 commits October 5, 2021 10:56

Use lodash/fp

a2a3a8a

Clarify types for EngineDetails fields

e0ed6a4

JasonStoltz approved these changes Oct 5, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into suggestions-callout

5840b55

Merge branch 'master' into suggestions-callout

91240ea

Merge branch 'master' into suggestions-callout

41d79ea

byronhulcher merged commit 8109e65 into elastic:master Oct 6, 2021

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 6, 2021

[App Search] Suggestions Callouts for Engine, Analytics, and Manual C…

bdce6b0

…uration Detail (elastic#113864)

kibanamachine mentioned this pull request Oct 6, 2021

[7.x] [App Search] Suggestions Callouts for Engine, Analytics, and Manual Curation Detail (#113864) #114051

Merged

kibanamachine added a commit that referenced this pull request Oct 6, 2021

[App Search] Suggestions Callouts for Engine, Analytics, and Manual C…

b0b406c

…uration Detail (#113864) (#114051) Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>

Conversation

byronhulcher commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byronhulcher commented Oct 5, 2021

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byronhulcher Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylersmalley commented Oct 6, 2021

Uh oh!

tylersmalley commented Oct 6, 2021

Uh oh!

kibanamachine commented Oct 6, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Oct 6, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

byronhulcher commented Oct 4, 2021 •

edited

Loading

byronhulcher Oct 5, 2021 •

edited

Loading