[App Search] Results follow-up #96709

cee-chen · 2021-04-09T18:27:14Z

Summary

I missed the boat for pushing up changes to #96184, so here's a follow up PR with some clean up items.

CSS cleanup: c4627d5
- Mostly involves deleting CSS that's no longer being used as of the App Search: Result Component Updates #96184 but wasn't removed
ResultActions refactor + DRYing out link behavior: f8cf5dd
- Create new separate ResultActions component
- Pass actions array through to header and have header in charge of conditional visibility / FlexItem wrapper (this matches the other header items)
- shouldLinkToDetailPage: instead of generating custom JSX, just have it be a standard action and append it to the actions array
- ResultHeaderItem - switch to EuiLinkTo, no need for extra wrapper
- ResultHeader - DRY out unnecessary extra path generation - instead pass down a conditional documentLink instead of a bool

QA

Tested documents view in a source engine + click behavior
Tested documents view in a meta engine + click behavior
Tested Relevance Tuning results in a source & meta engine
Tested Curations results in a source & meta engine
Tested with a screen reader (IMO, the new action behavior of the link/eye button is better now)

Checklist

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

- Create new separate ResultActions component - Pass actions array through to header and have haeder in charge of conditional visibility / FlexItem wrapper (this matches the other header items) - shouldLinkToDetailPage: instead of generating custom JSX, just have it be a standard action and append it to the actions array Link behavior: - ResultHeaderItem - switch to EuiLinkTo, no need for extra wrapper - ResultHeader - DRY out unnecessary extra path generation - instead pass down a conditional documentLink instead of a bool

cee-chen · 2021-04-09T18:28:27Z

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

-      expect(buttons).toHaveLength(2);
-
-      expect(buttons.first().prop('iconType')).toEqual('eyeClosed');
-      expect(buttons.first().prop('color')).toEqual('danger');
-      buttons.first().simulate('click');
-      expect(actions[0].onClick).toHaveBeenCalled();
-
-      expect(buttons.last().prop('iconType')).toEqual('starFilled');
-      // Note that no iconColor was passed so it was defaulted to primary
-      expect(buttons.last().prop('color')).toEqual('primary');
-      buttons.last().simulate('click');
-      expect(actions[1].onClick).toHaveBeenCalled();


Most of this got moved to the new result_actions.test.tsx

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

cee-chen · 2021-04-09T18:31:08Z

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

-          shouldLinkToDetailPage={shouldLinkToDetailPage}
-          actions={<ResultActions />}
+          documentLink={documentLink}
+          actions={actions}


I could have left this as actions={<ResultActions actions={actions} />} but moving <ResultActions> to ResultHeader made it easier to unit test without diving, and the conditional logic pattern there matched the rest of the conditional header items.

cee-chen · 2021-04-09T18:35:43Z

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

+    <header className="appSearchResultHeader">
+      <EuiFlexGroup
+        alignItems="center"
+        gutterSize="s"
+        justifyContent="spaceBetween"
+        responsive={false}
+        wrap
+      >


Responsive enhancements:

Before:

After:

Wrapping also generally just responds better on result cards that aren't on a smaller screen but are in a smaller column (e.g. Relevance Tuning).

cee-chen · 2021-04-09T18:35:57Z

...lugins/enterprise_search/public/applications/app_search/components/result/result_actions.tsx

+
+export const ResultActions: React.FC<Props> = ({ actions }) => {
+  return (
+    <EuiFlexGroup gutterSize="s" responsive={false}>


responsive={false} was added here. See above before/after screenshot

JasonStoltz · 2021-04-12T15:46:22Z

@elasticmachine merge upstream

daveyholler

Visually, LGTM. (Sorry for getting merge-happy on the other PR 😝)

cee-chen · 2021-04-12T16:04:06Z

Haha all good, no worries! Totally on me for not leaving a comment / skiving off that day 😆

JasonStoltz · 2021-04-12T16:08:36Z

...lugins/enterprise_search/public/applications/app_search/components/result/result_actions.tsx

+  actions: ResultAction[];
+}
+
+export const ResultActions: React.FC<Props> = ({ actions }) => {


Good call on moving this out to its own component, this simplifies things greatly.

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

JasonStoltz

LGTM, I recommend making that change to unshift though!

JasonStoltz · 2021-04-12T16:28:47Z

Great work on this BTW, thank you for taking the time to improve this.

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

cee-chen · 2021-04-12T17:09:58Z

@elasticmachine merge upstream

kibanamachine · 2021-04-12T19:12:27Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1323	1324	+1

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	1.9MB	-4.4KB

History

💚 Build #118825 succeeded 032fc94
💔 Build #118794 failed bd1384c
💚 Build #118552 succeeded f8cf5dd

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

* CSS cleanup * Refactor ResultActions component + DRY out link behavior - Create new separate ResultActions component - Pass actions array through to header and have haeder in charge of conditional visibility / FlexItem wrapper (this matches the other header items) - shouldLinkToDetailPage: instead of generating custom JSX, just have it be a standard action and append it to the actions array Link behavior: - ResultHeaderItem - switch to EuiLinkTo, no need for extra wrapper - ResultHeader - DRY out unnecessary extra path generation - instead pass down a conditional documentLink instead of a bool * PR feedback: Fix test name * PR feedback: unshift Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-04-12T19:15:56Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

* CSS cleanup * Refactor ResultActions component + DRY out link behavior - Create new separate ResultActions component - Pass actions array through to header and have haeder in charge of conditional visibility / FlexItem wrapper (this matches the other header items) - shouldLinkToDetailPage: instead of generating custom JSX, just have it be a standard action and append it to the actions array Link behavior: - ResultHeaderItem - switch to EuiLinkTo, no need for extra wrapper - ResultHeader - DRY out unnecessary extra path generation - instead pass down a conditional documentLink instead of a bool * PR feedback: Fix test name * PR feedback: unshift Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance <constancecchen@users.noreply.github.com>

cee-chen added 2 commits April 9, 2021 09:50

CSS cleanup

c4627d5

cee-chen added 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 9, 2021

cee-chen requested review from daveyholler, JasonStoltz and a team April 9, 2021 18:27

cee-chen commented Apr 9, 2021

View reviewed changes

Merge branch 'master' into results-cleanup

bd1384c

daveyholler approved these changes Apr 12, 2021

View reviewed changes

JasonStoltz reviewed Apr 12, 2021

View reviewed changes

...ns/enterprise_search/public/applications/app_search/components/result/result_header.test.tsx Outdated Show resolved Hide resolved

PR feedback: Fix test name

032fc94

JasonStoltz approved these changes Apr 12, 2021

View reviewed changes

cee-chen commented Apr 12, 2021

View reviewed changes

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

PR feedback: unshift

08d2798

cee-chen enabled auto-merge (squash) April 12, 2021 16:55

Merge branch 'master' into results-cleanup

f99baf3

cee-chen merged commit 171f398 into elastic:master Apr 12, 2021

kibanamachine mentioned this pull request Apr 12, 2021

[7.x] [App Search] Results follow-up (#96709) #96872

Merged

cee-chen deleted the results-cleanup branch April 12, 2021 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Results follow-up #96709

[App Search] Results follow-up #96709

cee-chen commented Apr 9, 2021

cee-chen Apr 9, 2021

cee-chen Apr 9, 2021

cee-chen Apr 9, 2021 •

edited

Loading

cee-chen Apr 9, 2021

JasonStoltz commented Apr 12, 2021

daveyholler left a comment

cee-chen commented Apr 12, 2021

JasonStoltz Apr 12, 2021 •

edited

Loading

JasonStoltz left a comment

JasonStoltz commented Apr 12, 2021

cee-chen commented Apr 12, 2021

kibanamachine commented Apr 12, 2021

kibanamachine commented Apr 12, 2021

[App Search] Results follow-up #96709

[App Search] Results follow-up #96709

Conversation

cee-chen commented Apr 9, 2021

Summary

QA

Checklist

cee-chen Apr 9, 2021

Choose a reason for hiding this comment

cee-chen Apr 9, 2021

Choose a reason for hiding this comment

cee-chen Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen Apr 9, 2021

Choose a reason for hiding this comment

JasonStoltz commented Apr 12, 2021

daveyholler left a comment

Choose a reason for hiding this comment

cee-chen commented Apr 12, 2021

JasonStoltz Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

JasonStoltz left a comment

Choose a reason for hiding this comment

JasonStoltz commented Apr 12, 2021

cee-chen commented Apr 12, 2021

kibanamachine commented Apr 12, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Apr 12, 2021

💚 Backport successful

cee-chen Apr 9, 2021 •

edited

Loading

JasonStoltz Apr 12, 2021 •

edited

Loading