[App Search] Convert Engine subnav and Engine Overview pages to new page template #102679

cee-chen · 2021-06-20T03:43:04Z

Summary

Follow up to #102170 - converts more App Search pages to the new KibanaPageTemplate. I'm attempting to break up the AS layout conversion into smaller, easier to review chunks.

This PR handles converting the Engine sub nav to an array of EuiSideNav items, and converts the original Engine Overview page as well so there's one view where the subnav can be seen. As always, follow along by commit (and turn off whitespace diffs).

Screencaps

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

- To facilitate this being broken up into multiple PRs, non-migrated views still get a wrapping layout

+ tweak CSS for new label - heads up that this will break the old nav, but will disappear as the pages get converted

cee-chen · 2021-06-20T03:47:43Z

...ugins/enterprise_search/public/applications/app_search/components/engine/engine_nav.test.tsx

 import { mockEngineValues } from '../../__mocks__';

+jest.mock('../../../shared/layout', () => ({
+  ...jest.requireActual('../../../shared/layout'), // TODO: Remove once side nav components are gone


The Layout and SideNav components will get deleted in one final PR (likely shared with WS and AS).

cee-chen · 2021-06-20T03:49:20Z

...ugins/enterprise_search/public/applications/app_search/components/engine/engine_nav.test.tsx

+describe('useEngineNav', () => {
+  const values = { ...mockEngineValues, myRole: {}, dataLoading: false };


This mostly copies the EngineNav test suite down below, it just converts the assertions from expecting react components to expecting EuiSideNavItem objects. (except for portions of the nav that render raw jsx)

cee-chen · 2021-06-20T03:56:44Z

...ck/plugins/enterprise_search/public/applications/app_search/components/engine/engine_nav.tsx

@@ -47,6 +55,255 @@ import { EngineLogic, generateEnginePath } from './';

 import './engine_nav.scss';

+export const useEngineNav = () => {


Again this is mostly just the existing EngineNav but converted to nav objects. It's not really possible in this branch to fully QA this (e.g. the nav icons), but I can push up a test branch with all pages converted (just not split out or any tests written) if folks really want it. Or we can wait til the end in any case, since this week is the week for QA passes 🤷

cee-chen · 2021-06-20T03:57:04Z

...plugins/enterprise_search/public/applications/app_search/components/engine/engine_router.tsx

@@ -85,74 +87,76 @@ export const EngineRouter: React.FC = () => {
  }

  const isLoadingNewEngine = engineName !== engineNameFromUrl;
-  if (isLoadingNewEngine || dataLoading) return <Loading />;
+  if (isLoadingNewEngine || dataLoading) return <AppSearchPageTemplate isLoading />;


This is a little odd-looking at first glance, but without it we get a flash of empty page when navigating between a non-engine and engine page. We also do need to wait for this data to be done loading since all subroutes expect top-level engine data to be present before their own logic begins.

FWIW, I had to do something similar for WS's Source views (#102592 (comment)).

cee-chen · 2021-06-20T04:05:44Z

...erprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx

-  if (dataLoading) {
-    return <Loading />;
-  }
+  if (dataLoading) return <AppSearchPageTemplate isLoading />;


I'm not a huge fan of this particular whole-page isLoading early return, but unfortunately with pages that have page titles jump change based on loaded data (in this case, we show either Engine overview or Engine setup depending on whether the engine has documents), it's better not to show the page at all initially - otherwise what happens is you get a brief flash of the Engine setup page which then jumps to Engine overview 🤷

WS has similar issues with their overview as well. Discussion for more context: #102592 (comment)

In this case, my ideal solution for this view would be to abstract the documentsCount / isEngineEmpty check to the top level engine logic available to all engine pages. That would allow the setup vs overview check to happen instantaneously, no data loading needed. I didn't want to add more lines into this PR though, so I left it out for now - time permitting, I'll follow up with some UX polish PRs this week.

Yeah I'd like to have a larger conversation about loading and how we handle it, I think we could improve the consistency of our loading patterns across the app for section routers, individual views, and individual componnennts. I agree it was better to limit this PRs scope

The new page template does actually improve consistent page-level loading patterns fairly significantly! I like that the page headers load in instantaneously, it makes navigation feel much snappier - it just backfires in scenarios where we conditionally change the entire page header/title heh (which we should consider potentially moving away from as a UX pattern).

Like you said though, there's always room to improve and think about loading individual components instead 👍

cee-chen · 2021-06-20T04:06:49Z

...rch/public/applications/app_search/components/engine_overview/engine_overview_empty.test.tsx

+    expect(getPageHeaderActions(wrapper).find(EuiButton).prop('href')).toEqual(
+      `${docLinks.appSearchBase}/index.html`


😎 new test_helpers already paying off in spades

byronhulcher

Looks good, one non-blocking comment

byronhulcher · 2021-06-21T14:35:01Z

...k/plugins/enterprise_search/public/applications/app_search/components/engine/engine_nav.scss

-    flex-direction: row;
-  }
+.appSearchNavIcon {
+  order: 1;


👀 👀 👀

Seeing order rules in CSS always raises the hairs on the back of my neck. I trust there's a good reason for this, but could you leave a small comment in the code explaining what that reason is?

Nice catch on this Byron! Kibana renders its icons to the left of text instead of to the right:

But we want the icons to the right, to match our previous UI:

If it had been more annoying than a simple CSS/flex re-order to do this I might have left them alone, but I figured since there's an easy way to quickly replicate our old nav icon UI/UX I might as well do so.

Also, if EUI or Davey pushes back on this / wants it removed, it's definitely easy enough to do so in the future!

Oh, just saw the request was for a code comment 🤦‍♀️ hope this looks good!

Suggested change

order: 1;

// EuiSideNav renders icons to the left of the nav link by default, but we use icons

// as warning or error indicators & prefer to render them on the right side of the nav

order: 1;

byronhulcher · 2021-06-21T14:46:56Z

...erprise_search/public/applications/app_search/components/engine_overview/engine_overview.tsx

-  if (dataLoading) {
-    return <Loading />;
-  }
+  if (dataLoading) return <AppSearchPageTemplate isLoading />;


Yeah I'd like to have a larger conversation about loading and how we handle it, I think we could improve the consistency of our loading patterns across the app for section routers, individual views, and individual componnennts. I agree it was better to limit this PRs scope

byronhulcher · 2021-06-21T14:49:05Z

@elasticmachine merge upstream

kibanamachine · 2021-06-21T18:53:11Z

💚 Build Succeeded

Metrics [docs]

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	+18.8KB

History

💚 Build #132759 succeeded 4c4ed34
💚 Build #132727 succeeded a1375ae
💚 Build #132522 succeeded 3f1618d

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

…age template (elastic#102679) * Update routers - To facilitate this being broken up into multiple PRs, non-migrated views still get a wrapping layout * Set up Engine subnav in EuiSideNav format + tweak CSS for new label - heads up that this will break the old nav, but will disappear as the pages get converted * Convert Engine Overview pages to new page template * [PR feedback] Code comment explaining side nav icon reorder Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-06-21T19:04:22Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…age template (#102679) (#102797) * Update routers - To facilitate this being broken up into multiple PRs, non-migrated views still get a wrapping layout * Set up Engine subnav in EuiSideNav format + tweak CSS for new label - heads up that this will break the old nav, but will disappear as the pages get converted * Convert Engine Overview pages to new page template * [PR feedback] Code comment explaining side nav icon reorder Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance <constancecchen@users.noreply.github.com>

cee-chen added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 20, 2021

cee-chen requested a review from a team June 20, 2021 03:43

cee-chen added 3 commits June 19, 2021 21:19

Update routers

6b336c1

- To facilitate this being broken up into multiple PRs, non-migrated views still get a wrapping layout

Set up Engine subnav in EuiSideNav format

3b03c4f

+ tweak CSS for new label - heads up that this will break the old nav, but will disappear as the pages get converted

Convert Engine Overview pages to new page template

3f1618d

cee-chen force-pushed the kibana-page-template-as-3 branch from 9dc3392 to 3f1618d Compare June 20, 2021 04:19

cee-chen commented Jun 20, 2021

View reviewed changes

byronhulcher approved these changes Jun 21, 2021

View reviewed changes

kibanamachine and others added 3 commits June 21, 2021 10:49

Merge branch 'master' into kibana-page-template-as-3

a1375ae

[PR feedback] Code comment explaining side nav icon reorder

4c4ed34

Merge branch 'master' into kibana-page-template-as-3

3371624

cee-chen merged commit 1f2de03 into elastic:master Jun 21, 2021

cee-chen deleted the kibana-page-template-as-3 branch June 21, 2021 19:00

kibanamachine mentioned this pull request Jun 21, 2021

[7.x] [App Search] Convert Engine subnav and Engine Overview pages to new page template (#102679) #102797

Merged

cee-chen mentioned this pull request Jun 23, 2021

[App Search] Refactor empty engine polling to EngineLogic #103041

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Convert Engine subnav and Engine Overview pages to new page template #102679

[App Search] Convert Engine subnav and Engine Overview pages to new page template #102679

cee-chen commented Jun 20, 2021

cee-chen Jun 20, 2021

cee-chen Jun 20, 2021

cee-chen Jun 20, 2021

cee-chen Jun 20, 2021

cee-chen Jun 20, 2021

byronhulcher Jun 21, 2021

cee-chen Jun 21, 2021

cee-chen Jun 20, 2021

byronhulcher left a comment

byronhulcher Jun 21, 2021

cee-chen Jun 21, 2021 •

edited

Loading

cee-chen Jun 21, 2021

cee-chen Jun 21, 2021 •

edited

Loading

byronhulcher Jun 21, 2021

byronhulcher commented Jun 21, 2021

kibanamachine commented Jun 21, 2021

kibanamachine commented Jun 21, 2021

		describe('useEngineNav', () => {
		const values = { ...mockEngineValues, myRole: {}, dataLoading: false };

		@@ -47,6 +55,255 @@ import { EngineLogic, generateEnginePath } from './';

		import './engine_nav.scss';

		export const useEngineNav = () => {

		expect(getPageHeaderActions(wrapper).find(EuiButton).prop('href')).toEqual(
		`${docLinks.appSearchBase}/index.html`

[App Search] Convert Engine subnav and Engine Overview pages to new page template #102679

[App Search] Convert Engine subnav and Engine Overview pages to new page template #102679

Conversation

cee-chen commented Jun 20, 2021

Summary

Screencaps

Checklist

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

byronhulcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jun 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byronhulcher commented Jun 21, 2021

kibanamachine commented Jun 21, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Jun 21, 2021

💚 Backport successful

cee-chen Jun 21, 2021 •

edited

Loading

cee-chen Jun 21, 2021 •

edited

Loading