[Enterprise Search] Add solution-level side navigation#74705
[Enterprise Search] Add solution-level side navigation#74705cee-chen merged 10 commits intoelastic:masterfrom
Conversation
|
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
left a comment
There was a problem hiding this comment.
Great work as always! Have a few nits and suggestions
x-pack/plugins/enterprise_search/public/applications/shared/layout/layout.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/enterprise_search/public/applications/shared/layout/side_nav.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Could use the same legacyAppSearchUrl recommended in prev comment
There was a problem hiding this comment.
Per above comment, will address this later in a routes constants file refactor
There was a problem hiding this comment.
Seems like a bunch of duplicated code with the switch and setup guide routes. Could this be refactored?
There was a problem hiding this comment.
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)
172d274 to
b97c75e
Compare
|
Tested in the following browsers:
Tested with the following accessibility tools:
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:
|
- 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
|
Marking this PR as ready for review after finishing a last round of testing + design pass with @daveyholler 🎉 |
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
History
To update your PR or re-run it, just comment with: |
|
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) ...
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