-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enterprise Search] Support active nav links that have both subnav & non-subnav child routes #103036
Conversation
@@ -18,7 +18,7 @@ export const CREDENTIALS_PATH = '/credentials'; | |||
export const ROLE_MAPPINGS_PATH = '/role_mappings'; | |||
|
|||
export const ENGINES_PATH = '/engines'; | |||
export const ENGINE_CREATION_PATH = '/engine_creation'; | |||
export const ENGINE_CREATION_PATH = `${ENGINES_PATH}/new`; // This is safe from conflicting with an :engineName path because new is a reserved name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[AS] /engines/new
is the same route used in the standalone UI
@@ -39,7 +39,7 @@ export const ENGINE_REINDEX_JOB_PATH = `${ENGINE_SCHEMA_PATH}/reindex_job/:reind | |||
export const ENGINE_CRAWLER_PATH = `${ENGINE_PATH}/crawler`; | |||
export const ENGINE_CRAWLER_DOMAIN_PATH = `${ENGINE_CRAWLER_PATH}/domains/:domainId`; | |||
|
|||
export const META_ENGINE_CREATION_PATH = '/meta_engine_creation'; | |||
export const META_ENGINE_CREATION_PATH = `${ENGINES_PATH}/new_meta_engine`; // This is safe from conflicting with an :engineName path because engine names cannot have underscores |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[AS] This is not the same route as the old standalone UI - I think it was /meta_engines/new
there.
I'm not 100% sold on /engines/new_meta_engine
myself - do folks prefer something more like /engines/new/meta_engine
?
@@ -8,7 +8,7 @@ | |||
import { setMockValues } from '../../../__mocks__/kea_logic'; | |||
|
|||
jest.mock('../../../shared/layout', () => ({ | |||
generateNavLink: jest.fn(({ to }) => ({ href: to })), | |||
generateNavLink: jest.fn(({ to, items }) => ({ href: to, items })), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about DRYing out an importable mock for shared/layout/generateNavLink
at some point since a few different nav files use it, but might save that for the final page template PR
@elasticmachine merge upstream |
^ Master has a broken commit. Nothing I can do until it gets reverted by devops |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM! Thanks for knocking this out and for the great, easy to read refactor 💯
…ine isSelected + change getNavLinkActive to early returns + tweak tests for readability
- to show active on creation routes but not on single source routes
- should eventually show active on creation routes but not on single engine routes
- so that it correctly shows as a child route of the Engines link + update breadcrumbs
bf437ad
to
31dab28
Compare
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Firefox XPack UI Functional Tests.x-pack/test/functional/apps/grok_debugger/grok_debugger·js.logstash grok debugger app "before all" hook in "grok debugger app"Standard Out
Stack Trace
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Thanks y'all! |
…non-subnav child routes (elastic#103036) * Update generateNavlink to take an `items` subNav and use it to determine isSelected + change getNavLinkActive to early returns + tweak tests for readability * Update WS nav Sources link - to show active on creation routes but not on single source routes * Update AS nav Engines link - should eventually show active on creation routes but not on single engine routes * Update AS engine creation routing - so that it correctly shows as a child route of the Engines link + update breadcrumbs
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…non-subnav child routes (#103036) (#103142) * Update generateNavlink to take an `items` subNav and use it to determine isSelected + change getNavLinkActive to early returns + tweak tests for readability * Update WS nav Sources link - to show active on creation routes but not on single source routes * Update AS nav Engines link - should eventually show active on creation routes but not on single engine routes * Update AS engine creation routing - so that it correctly shows as a child route of the Engines link + update breadcrumbs Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Summary
This functionality was requested by Vadim in #102592 (comment), but it benefits both Workplace Search (Sources) and App Search (Engines).
In the case of both WS Sources and AS Engines, their side nav links have a subnav (the
items
array) for single Source or single Engine navigation, but it also has a "Create Source"/"Create Engine" link or route of some sort that should not show a subnav, but should still highlight the parent Sources/Engines link as active.The problem prior to this PR is that we cannot simply throw on an
shouldShowActiveForSubroutes
flag, because the parent link shouldn't show active for subroutes while its subnav is open/has active links:Left: Behavior with only
shouldShowActiveForSubroutes
; Right: how EUI demonstrates active nested linksThis PR addresses that problem by passing the
items
subnav into ourgenerateNavLink
helper, which then usesitems
to determine whether or not the nav link should be shown as active (isSelected
) or not.Screencaps
Workplace Search
Before
After
App Search
Before
After
Checklist