[App Search] Add a Result Component #85046

JasonStoltz · 2020-12-04T17:30:22Z

Summary

This PR adds a reusable Result component, which will ultimately be leveraged in a number of locations in the UI including; Documents, Curations, Query Tester, and Relevance Tuning.

This was migrated from the StuiResult component in ent-search.

This PR primarily focuses on the Documents view, and migrating ONLY what is required by the Documents view. (There is only one exception to this, and that is the inclusion of the "score" in the ResultHeader component, which won't be used for the Documents view).

Things that were not migrated over that we will need to migrate in follow up PRs for the Documents view, but were left out to keep this PR small:

Field type and snippet highlighting, which will require us to fetch the engine schema.
Linking to a detail page.

Things that were not migrated over that we will need to migrate later for other views:

The condensed option, which appears to be used in Query Tester, Curations, and Relevance Tuning
Support for showing boosts, which will show which fields have been boosted and will be used on Relevance Tuning
Support for searchFields, which will show which fields have weights applied and will be used on Relevance Tuning
extra action support, which lets us add an additional option to the right side of the Result element, and will be used on Curations
unsearched fields support, which will show unsearched fields, like 'number' and 'date' field types below other fields. This may be required on the Relevance Tuning page.

Things that were intentionally not migrated, and won't be migrated:

Click tracking on the result component, when clicking into the Document Details page. It was decided that we will no longer track Dashboard actions in Analytics with the migration to Kibana.
Many styles - this PR tried to pull in only the minimum number of styles required.
A tool tip that used to show when hovering over truncated Engine names and IDs in the Result Header. We instead rely on the title attribute for simplicity.
unsearched fields support on the Documents view, as we decided that showing which fields are not searched is confusing to see on the Documents page

The Library Page

This component has many permutations, and it's very difficult to develop a component like this without visually seeing all of the permutations in a single place. Something like a styleguide or storybook is often used for this. In lieu of a full blown styleguide, I added a simple /library page to App Search, accessible in dev mode only, which shows all of the variations of this component.

We DO NOT need to keep it if this is a bad idea. This was a helpful development aid, and will be helpful in the future as we start to add more states to this component. That being said, we don't need to keep it, and I can easily delete it at this point.

Checklist

Delete any items that are not applicable to this PR.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

cee-chen

First pass of comments - mostly a quick look at types & some SCSS, will dig more deeply in later!

x-pack/plugins/enterprise_search/public/applications/shared/types.ts

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

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.scss

...lugins/enterprise_search/public/applications/app_search/components/result/result_header.scss

...s/enterprise_search/public/applications/app_search/components/result/result_header_item.scss

...plugins/enterprise_search/public/applications/app_search/components/result/result_field.scss

Co-authored-by: Constance <[email protected]>

into documents-view-4

JasonStoltz · 2020-12-07T17:00:36Z

@constancecchen Ready for a second look

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx

...plugins/enterprise_search/public/applications/app_search/components/result/result_field.scss

...lugins/enterprise_search/public/applications/app_search/components/result/result_header.scss

cee-chen

Apologies that my comments are coming in so piecemeal - I'm super scattered/not very focused today so I figured I'd just try to give you stuff to work on as I go instead of a huge dump at the end of the day.

Just assume I'm still slowly working my way through QA/files until I make a comment saying "final set of comments" or something similar 😅

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.scss

...lugins/enterprise_search/public/applications/app_search/components/result/result_header.scss

- remove display grid on contentWrap - remove grid on contentInner - remove unnecessary fieldsetContainer wrapper - remove extra blank div - remove unnecessary CSS on .appSearchResult

…naming (apologies for going back and forth on this)

cee-chen

This comment set contains:

Some misc naming simplification suggestions, now that we've removed some extra wrappers
Some semantic markup suggestions

Full diff can be checked here for ease: cee-chen@dda58c4

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.scss

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx

...plugins/enterprise_search/public/applications/app_search/components/result/result_header.tsx

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx

cee-chen

Misc comment set: Couple of unnecessary key props, extra span, an accessibility suggestion

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx

.../plugins/enterprise_search/public/applications/app_search/components/result/result_field.tsx

...ns/enterprise_search/public/applications/app_search/components/result/result_header_item.tsx

.../plugins/enterprise_search/public/applications/app_search/components/result/result_field.tsx

cee-chen

After doing some responsive testing at 320px device width, I'd like to propose a change/simplification to the ResultField CSS file that should look better on mobile and look mostly similar on larger views:

Mobile 320px

Before (left), After (right)

Notice that the after has much more readable content

Standard / 700px (should not have much change)

Before (top), After (bottom)

...plugins/enterprise_search/public/applications/app_search/components/result/result_field.scss

cee-chen · 2020-12-08T01:08:14Z

I have another set of proposed mobile responsive changes for ResultHeader here - unfortunately it's tougher to add code line comments/suggestions for those since they rely on changes not yet made / in my above comments. So instead, here's the diff:

cee-chen@a2573bd#diff-3e0fecf720a0b1119904329cd03c897a75961a7a9959e79881cb9ff08c7ee43b

And here's screencaps of the visual changes:

Before

After

Just IMO, the after is a little more organized/tidier and also better handles long overflowing IDs - lmk what you think!

cee-chen · 2020-12-08T01:10:08Z

OK, sorry, I know that was a lot :) That was my final set of DOM/CSS comments (mostly from inspecting the browser & doing testing) - I haven't yet reviewed any JS logic as a heads up, I'll probably have to do that tomorrow.

I have all (I think) of the above changes in code diffs if you'd prefer to cherry-pick them for speed - no worries if not and you'd rather review each comment individually: JasonStoltz/kibana@documents-view-4...constancecchen:documents-view-4

Documents view 4

JasonStoltz · 2020-12-08T18:34:22Z

Looks great @constancecchen! Really handy that you put this into a branch, it made it easy to apply your changes. This clean this up quite a bit, thank you! 👍 👍

JasonStoltz · 2020-12-08T18:34:51Z

I think your branch had everything, I don't think I missed any of your comments.

x-pack/plugins/enterprise_search/public/applications/app_search/components/library/library.tsx

cee-chen

Just a few small nit comments (that I missed in previous passes, totally on me) and one perf suggestion - everything looks amazing and this is my actual last set of comments! 🙇‍♀️

...ns/enterprise_search/public/applications/app_search/components/result/result_header_item.tsx

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx

...ns/enterprise_search/public/applications/app_search/components/result/result_header_item.tsx

...terprise_search/public/applications/app_search/components/result/result_header_item.test.tsx

...ins/enterprise_search/public/applications/app_search/components/result/result_field.test.tsx

cee-chen · 2020-12-08T19:44:03Z

...ns/enterprise_search/public/applications/app_search/components/result/result_header.test.tsx

+      expect(wrapper.find('[data-test-subj="ResultEngine"]').prop('value')).toBe('my-engine');
+    });
+
+    it('does not render an engine name if the ids match, which means it is not a meta engine', () => {


[not a change request, just me thinking out loud] I wonder if it would it be potentially possible to pass an isMetaEngine var directly to the Result view, so that we don't have to make a manual check per-result ourselves? We have isMetaEngine is EngineLogic, just not sure if it applies to the Result view

I think you are right, that would definitely have worked.

...ns/enterprise_search/public/applications/app_search/components/result/result_header.test.tsx

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx

...k/plugins/enterprise_search/public/applications/app_search/components/result/result.test.tsx

Co-authored-by: Constance <[email protected]>

cee-chen · 2020-12-09T19:14:20Z

@elasticmachine merge upstream

cee-chen

Thanks so much as always for your patience and awesome work Jason! I can't say enough how excited I am for this super clean Result view and the awesome library/storybook pattern to potentially reuse for the future!

kibanamachine · 2020-12-09T20:58:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 899042c

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	969	1011	+42

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.7MB	1.7MB	+32.2KB

Distributable file count

id	before	after	diff
`default`	46984	47748	+764

History

💔 Build #93213 failed fdc7cf2
💛 Build #92852 was flaky 843284f
💔 Build #92838 failed 128b4ce
💚 Build #92494 succeeded 2ccd75c
💚 Build #92425 succeeded ff3d9fb

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

kibanamachine · 2020-12-15T19:04:40Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

JasonStoltz added 2 commits December 4, 2020 11:53

Renamed result_field_value directory

29d85d5

Added Library

d662fd0

JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Dec 4, 2020

JasonStoltz requested review from a team December 4, 2020 17:30

JasonStoltz added 2 commits December 4, 2020 13:57

Added Result component

659d4b1

Added component ResultHeader

04b586e

JasonStoltz force-pushed the documents-view-4 branch from f26ea4e to 04b586e Compare December 4, 2020 18:58

cee-chen reviewed Dec 4, 2020

View reviewed changes

JasonStoltz and others added 9 commits December 7, 2020 11:10

Apply suggestions from code review

1270bf7

Co-authored-by: Constance <[email protected]>

Remove container

ace583e

Remove double focus

df08eaf

Remove outline none

88eddf3

Merge branch 'documents-view-4' of https://github.com/JasonStoltz/kibana

f954abe

into documents-view-4

Less precise sizes

68f9780

Unnest column

b9e1126

removed generic div selectors

375ecef

Merge branch 'master' into documents-view-4

ff3d9fb

JasonStoltz requested a review from cee-chen December 7, 2020 17:00

cee-chen reviewed Dec 7, 2020

View reviewed changes

x-pack/plugins/enterprise_search/public/applications/app_search/components/result/result.tsx Outdated Show resolved Hide resolved

...plugins/enterprise_search/public/applications/app_search/components/result/result_field.scss Outdated Show resolved Hide resolved

cee-chen reviewed Dec 7, 2020

View reviewed changes

...lugins/enterprise_search/public/applications/app_search/components/result/result_header.scss Outdated Show resolved Hide resolved

Remove &

2ccd75c

JasonStoltz requested a review from cee-chen December 7, 2020 19:37

cee-chen reviewed Dec 7, 2020

View reviewed changes

...lugins/enterprise_search/public/applications/app_search/components/result/result_header.scss Outdated Show resolved Hide resolved

cee-chen added 3 commits December 7, 2020 15:05

Button cleanup

d46f655

Remove unnecessary display grids & wrappers

ecc2365

- remove display grid on contentWrap - remove grid on contentInner - remove unnecessary fieldsetContainer wrapper - remove extra blank div - remove unnecessary CSS on .appSearchResult

ResultHeader - remove unnecessary --left/--right modifiers + fix BEM …

6a6ff41

…naming (apologies for going back and forth on this)

cee-chen reviewed Dec 8, 2020

View reviewed changes

Merge pull request #4 from constancecchen/documents-view-4

128b4ce

Documents view 4

JasonStoltz requested a review from cee-chen December 8, 2020 18:34

Fix specs

843284f

richkuz reviewed Dec 8, 2020

View reviewed changes

x-pack/plugins/enterprise_search/public/applications/app_search/components/library/library.tsx Show resolved Hide resolved

cee-chen reviewed Dec 8, 2020

View reviewed changes

cee-chen mentioned this pull request Dec 9, 2020

[App Search] Documents view: misc responsive tweaks #85440

Merged

1 task

JasonStoltz and others added 2 commits December 9, 2020 13:19

Apply suggestions from code review

0a86dc7

Co-authored-by: Constance <[email protected]>

Fixed spec

fdc7cf2

JasonStoltz requested a review from cee-chen December 9, 2020 18:22

Merge branch 'master' into documents-view-4

899042c

cee-chen approved these changes Dec 9, 2020

View reviewed changes

JasonStoltz merged commit 31a1cc0 into elastic:master Dec 11, 2020

JasonStoltz deleted the documents-view-4 branch December 11, 2020 19:00

JasonStoltz mentioned this pull request Dec 15, 2020

[7.x] [App Search] Add a Result Component (#85046) #85961

Closed

JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Dec 15, 2020

[App Search] Add a Result Component (elastic#85046)

90c8831

JasonStoltz mentioned this pull request Dec 15, 2020

[7.x] [App Search] Add a Result Component (#85046) #85978

Merged

JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Dec 15, 2020

[App Search] Add a Result Component (elastic#85046)

5635414

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 15, 2020

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 15, 2020

JasonStoltz added a commit that referenced this pull request Dec 15, 2020

[App Search] Add a Result Component (#85046) (#85978)

2b04d66

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Add a Result Component #85046

[App Search] Add a Result Component #85046

JasonStoltz commented Dec 4, 2020

cee-chen left a comment

JasonStoltz commented Dec 7, 2020

cee-chen left a comment

cee-chen left a comment

cee-chen left a comment

cee-chen left a comment •

edited

Loading

cee-chen commented Dec 8, 2020

cee-chen commented Dec 8, 2020 •

edited

Loading

JasonStoltz commented Dec 8, 2020

JasonStoltz commented Dec 8, 2020

cee-chen left a comment •

edited

Loading

cee-chen Dec 8, 2020 •

edited

Loading

JasonStoltz Dec 9, 2020

cee-chen commented Dec 9, 2020

cee-chen left a comment

kibanamachine commented Dec 9, 2020

kibanamachine commented Dec 15, 2020

[App Search] Add a Result Component #85046

[App Search] Add a Result Component #85046

Conversation

JasonStoltz commented Dec 4, 2020

Summary

The Library Page

Checklist

For maintainers

cee-chen left a comment

Choose a reason for hiding this comment

JasonStoltz commented Dec 7, 2020

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen left a comment • edited Loading

Choose a reason for hiding this comment

Mobile 320px

Standard / 700px (should not have much change)

cee-chen commented Dec 8, 2020

Before

After

cee-chen commented Dec 8, 2020 • edited Loading

JasonStoltz commented Dec 8, 2020

JasonStoltz commented Dec 8, 2020

cee-chen left a comment • edited Loading

Choose a reason for hiding this comment

cee-chen Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

JasonStoltz Dec 9, 2020

Choose a reason for hiding this comment

cee-chen commented Dec 9, 2020

cee-chen left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 9, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

History

kibanamachine commented Dec 15, 2020

cee-chen left a comment •

edited

Loading

cee-chen commented Dec 8, 2020 •

edited

Loading

cee-chen left a comment •

edited

Loading

cee-chen Dec 8, 2020 •

edited

Loading