[Enterprise Search] Add solution-level side navigation #74705

cee-chen · 2020-08-10T20:12:27Z

Summary

This adds a solution-level layout & navigation component that both App Search and Workplace Search can use for their sidebar navigation:

Currently all links except the existing 'overview' pages simply link out externally/in a new tab to Enterprise Search.

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
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility
Tested in dark mode

cee-chen · 2020-08-10T20:13:52Z

This PR is currently WIP because we're still working on responsive/mobile behavior + accessibility/cross-browser testing needs to be done. However, I wanted to open this PR so that @scottybollinger could start taking a look at the code changes for review whenever (strongly recommend following along by commit if possible!)

EDIT: Scotty, if you'd like to start the Workplace Search nav sooner rather than later, I added a branch here that you can use: https://github.com/constancecchen/kibana/commits/ws-nav (last two commits are the ones to look at)

scottybollinger

Great work as always! Have a few nits and suggestions

x-pack/plugins/enterprise_search/public/applications/shared/layout/layout.tsx

x-pack/plugins/enterprise_search/public/applications/shared/layout/side_nav.tsx

scottybollinger · 2020-08-10T21:29:33Z

x-pack/plugins/enterprise_search/public/applications/app_search/index.tsx

      <Route exact path="/">
-        {!enterpriseSearchUrl ? <Redirect to="/setup_guide" /> : <EngineOverview />}
+        {!enterpriseSearchUrl ? <Redirect to="/setup_guide" /> : <Redirect to="/engines" />}


I think it would be good to go ahead and port over app/javascript/app_search/utils/routePaths.ts as referenced in previous comment. Already see where we could DRY out below

Ya I've been lazy about route strings vs constants up until now - I'm leaning towards doing that in a separate PR though, since this one already has so many lines/moving parts

scottybollinger · 2020-08-10T21:30:51Z

x-pack/plugins/enterprise_search/public/applications/app_search/index.tsx

+          defaultMessage: 'Engines',
+        })}
+      </SideNavLink>
+      <SideNavLink isExternal to={`${externalUrl}/settings/account`}>


Could use the same legacyAppSearchUrl recommended in prev comment

Per above comment, will address this later in a routes constants file refactor

scottybollinger · 2020-08-10T21:35:01Z

x-pack/plugins/enterprise_search/public/applications/app_search/index.tsx

+  if (!enterpriseSearchUrl)
+    return (
+      <Switch>
+        <Route exact path="/setup_guide">
+          <SetupGuide />
+        </Route>
+        <Route>
+          <Redirect to="/setup_guide" />
+          <SetupGuide />
+        </Route>
+      </Switch>
+    );

  return (
    <Switch>
-      <Route exact path="/">
-        {!enterpriseSearchUrl ? <Redirect to="/setup_guide" /> : <Redirect to="/engines" />}
-      </Route>
      <Route exact path="/setup_guide">
        <SetupGuide />
      </Route>
      <Route>
        <Layout navigation={<AppSearchNav />}>
          <Switch>
+            <Route exact path="/">
+              <EngineOverview />
+            </Route>


Seems like a bunch of duplicated code with the switch and setup guide routes. Could this be refactored?

It could be, but I don't think it's super necessary. I've already spent several hours trying to wrangle React Router in e94f8ff - the / root redirects unfortunately behave differently in Kibana than it does in ent-search (I think because we inherit history from Kibana?) and I don't want to overcomplicate it. I'd rather be extremely explicit about what React Router is doing than try to be clever/hide code to save 5-10 lines 🤷

That being said, maybe I'm not really fully seeing what refactor you mean? Feel free to give it a shot yourself if you want (just be very sure to test the /setup_guide URL and clicking the "App Search" breadcrumb back to the / index - that's where it tends to break)

cee-chen · 2020-08-11T20:55:52Z

Tested in the following browsers:

Tested with the following accessibility tools:

Mac Voiceover in Chrome

KNOWN ACCESSIBILITY ISSUE(ish):

There is a slight UX issue currently where screenreader & keyboard users on mobile/responsive views can tab to nav links that are "hidden" even if the nav is collapsed. I'm going to leave this as-is for now because:

The alternative is to add a LOT more JS logic/overhead in the form of a window resize listener like EuiCollapsibleNav does in order to conditionally hide the nav children, and I'd rather leave that to EUI to do if/when they decide to use this component
I imagine that most keyboard/screenreader users are on larger (>990px) screens in any case, and as such this feels like an edge case/the intersection of users that this affects is narrow. Not to say that I don't think it shouldn't still be fixed, but since the app still functions and links can still be navigated to, I'm making a conscious choice to pay this down later as tech debt.

- note: we should *not* set left: 0 / top: 0 etc., as this can interfere with Kibana's existing UI (e.g. docked navigation, telemetry callout)

- So that Workplace Search can reuse the same layout but pass in their own custom nav + Refactor AppSearch to use Layout in router

- If enterpriseSearchUrl hasn't been set, all pages should redirect to SetupGuide, not just root - The engines redirect simply wasn't working at all - it would always show a blank page when '/' was clicked in the Kibana breadcrumbs. Not sure if this is a Kibana issue - had to change to a component load to fix + Simplify index.test.tsx (probably unreasonable and not super helpful to add assertions for each new route)

- By adding a new useLocation mock + add SideNavLink active class test TODO: I should probably rename react_router_history.mock to just react_router.mock

- This requires updating our EUI/React Router components to accept and run custom onClick events - Also requires adding a new ReactContext to pass down closeNavigation, but that's not too onerous thanks to useContext

cee-chen · 2020-08-11T22:18:50Z

Marking this PR as ready for review after finishing a last round of testing + design pass with @daveyholler 🎉

kibanamachine · 2020-08-12T00:14:58Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e63d2b2

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
enterpriseSearch	191	+24	167

async chunks size

id	value	diff	baseline
enterpriseSearch	307.7KB	+40.6KB	267.1KB

History

💚 Build #67588 succeeded b97c75e66712c563771ff3b9a787b84ee83dc5c5
💚 Build #67407 succeeded 172d274d9f7267ab61439d66435d428133131bef
💚 Build #67366 succeeded ad92a1d0b99e8bbd110a49600497f780f840b51d

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

cee-chen · 2020-08-12T16:33:04Z

Thanks Scotty! 🙇‍♀️

* Add basic layout/sidebar blocking - note: we should *not* set left: 0 / top: 0 etc., as this can interfere with Kibana's existing UI (e.g. docked navigation, telemetry callout) * Add sidebar styles + static links * Refactor SideNav to be a reusable component - So that Workplace Search can reuse the same layout but pass in their own custom nav + Refactor AppSearch to use Layout in router * Refactor all views to account for in-router Layout * Fix root redirects not working as expected - If enterpriseSearchUrl hasn't been set, all pages should redirect to SetupGuide, not just root - The engines redirect simply wasn't working at all - it would always show a blank page when '/' was clicked in the Kibana breadcrumbs. Not sure if this is a Kibana issue - had to change to a component load to fix + Simplify index.test.tsx (probably unreasonable and not super helpful to add assertions for each new route) * Implement active styling for links * Fix failing location tests - By adding a new useLocation mock + add SideNavLink active class test TODO: I should probably rename react_router_history.mock to just react_router.mock * Add responsive toggle + styling * Add navigation accessibility attributes/controls * [Feedback] Update mobile UX to close menu on link click/navigation - This requires updating our EUI/React Router components to accept and run custom onClick events - Also requires adding a new ReactContext to pass down closeNavigation, but that's not too onerous thanks to useContext

* master: (28 commits) [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606) [Security Solution] Fix the status of timelines' bulk actions (elastic#74560) Data plugin: Suggested enhance pattern (elastic#74505) Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642) [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815) [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869) [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582) [bin/kibana-plugin] support KP plugins instead (elastic#74604) Reduce number of indexed fields in index pattern saved object (elastic#74817) [reporting] Pass along generic parameters in high-order route handler (elastic#74892) Migrated last pieces of legacy fixture code (elastic#74470) Empty index patterns page re-design (elastic#68819) [babel] coalese some versions to prevent breaking yarn install (elastic#74864) [Dashboard First] Decouple Attribute Service and By Value Embeddables (elastic#74302) Revert "[reporting] Pass along generic parameters in high-order route handler" (elastic#74891) [reporting] Pass along generic parameters in high-order route handler (elastic#74879) [src/dev/build] implement a getBuildNumber() mock (elastic#74881) [Enterprise Search] Add solution-level side navigation (elastic#74705) [DOCS] Canvas docs 7.9 refresh (elastic#74000) [Security Solution][Resolver]Enzyme test related events closing (elastic#74811) ...

…le-buffer-with-update-of-same-id * upstream/master: (37 commits) [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606) [Security Solution] Fix the status of timelines' bulk actions (elastic#74560) Data plugin: Suggested enhance pattern (elastic#74505) Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642) [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815) [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869) [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582) [bin/kibana-plugin] support KP plugins instead (elastic#74604) Reduce number of indexed fields in index pattern saved object (elastic#74817) [reporting] Pass along generic parameters in high-order route handler (elastic#74892) Migrated last pieces of legacy fixture code (elastic#74470) Empty index patterns page re-design (elastic#68819) [babel] coalese some versions to prevent breaking yarn install (elastic#74864) [Dashboard First] Decouple Attribute Service and By Value Embeddables (elastic#74302) Revert "[reporting] Pass along generic parameters in high-order route handler" (elastic#74891) [reporting] Pass along generic parameters in high-order route handler (elastic#74879) [src/dev/build] implement a getBuildNumber() mock (elastic#74881) [Enterprise Search] Add solution-level side navigation (elastic#74705) [DOCS] Canvas docs 7.9 refresh (elastic#74000) [Security Solution][Resolver]Enzyme test related events closing (elastic#74811) ...

cee-chen added release_note:enhancement Feature:Plugins v7.10.0 labels Aug 10, 2020

cee-chen requested review from daveyholler and scottybollinger August 10, 2020 20:12

scottybollinger reviewed Aug 10, 2020

View reviewed changes

cee-chen force-pushed the nav branch 2 times, most recently from 172d274 to b97c75e Compare August 11, 2020 19:58

cee-chen added 10 commits August 11, 2020 15:16

Add basic layout/sidebar blocking

4f6cb95

- note: we should *not* set left: 0 / top: 0 etc., as this can interfere with Kibana's existing UI (e.g. docked navigation, telemetry callout)

Add sidebar styles + static links

b9b38ad

Refactor SideNav to be a reusable component

cee4072

- So that Workplace Search can reuse the same layout but pass in their own custom nav + Refactor AppSearch to use Layout in router

Refactor all views to account for in-router Layout

dd5ec40

Implement active styling for links

f1206c2

Fix failing location tests

c3e6cc1

- By adding a new useLocation mock + add SideNavLink active class test TODO: I should probably rename react_router_history.mock to just react_router.mock

Add responsive toggle + styling

3a5e7fb

Add navigation accessibility attributes/controls

9f371ad

[Feedback] Update mobile UX to close menu on link click/navigation

e63d2b2

- This requires updating our EUI/React Router components to accept and run custom onClick events - Also requires adding a new ReactContext to pass down closeNavigation, but that's not too onerous thanks to useContext

cee-chen force-pushed the nav branch from b97c75e to e63d2b2 Compare August 11, 2020 22:18

cee-chen marked this pull request as ready for review August 11, 2020 22:20

cee-chen requested a review from scottybollinger August 12, 2020 14:40

scottybollinger approved these changes Aug 12, 2020

View reviewed changes

cee-chen merged commit 6ee2460 into elastic:master Aug 12, 2020

cee-chen deleted the nav branch August 12, 2020 16:33

cee-chen mentioned this pull request Aug 12, 2020

[7.x] [Enterprise Search] Add solution-level side navigation (#74705) #74882

Merged

cee-chen mentioned this pull request Aug 14, 2020

[Enterprise Search][tech debt] Refactor external URL generation to a single DRY helper #75093

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Search] Add solution-level side navigation #74705

[Enterprise Search] Add solution-level side navigation #74705

cee-chen commented Aug 10, 2020 •

edited

Loading

cee-chen commented Aug 10, 2020 •

edited

Loading

scottybollinger left a comment

scottybollinger Aug 10, 2020

cee-chen Aug 10, 2020

scottybollinger Aug 10, 2020

cee-chen Aug 10, 2020 •

edited

Loading

scottybollinger Aug 10, 2020

cee-chen Aug 10, 2020

cee-chen commented Aug 11, 2020 •

edited

Loading

cee-chen commented Aug 11, 2020

kibanamachine commented Aug 12, 2020

cee-chen commented Aug 12, 2020

[Enterprise Search] Add solution-level side navigation #74705

[Enterprise Search] Add solution-level side navigation #74705

Conversation

cee-chen commented Aug 10, 2020 • edited Loading

Summary

Checklist

cee-chen commented Aug 10, 2020 • edited Loading

scottybollinger left a comment

Choose a reason for hiding this comment

scottybollinger Aug 10, 2020

Choose a reason for hiding this comment

cee-chen Aug 10, 2020

Choose a reason for hiding this comment

scottybollinger Aug 10, 2020

Choose a reason for hiding this comment

cee-chen Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

scottybollinger Aug 10, 2020

Choose a reason for hiding this comment

cee-chen Aug 10, 2020

Choose a reason for hiding this comment

cee-chen commented Aug 11, 2020 • edited Loading

cee-chen commented Aug 11, 2020

kibanamachine commented Aug 12, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

async chunks size

History

cee-chen commented Aug 12, 2020

cee-chen commented Aug 10, 2020 •

edited

Loading

cee-chen commented Aug 10, 2020 •

edited

Loading

cee-chen Aug 10, 2020 •

edited

Loading

cee-chen commented Aug 11, 2020 •

edited

Loading