[Enterprise Search] Support active nav links that have both subnav & non-subnav child routes #103036
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cee-chen
added
release_note:skip
cee-chen
commented
Jun 23, 2021
| @@ -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.
[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.
[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.
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 |
scottybollinger
approved these changes
Jun 23, 2021
…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
cee-chen
force-pushed
the
nav-active-tweak
branch
from
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 OutStack TraceMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
|
Thanks y'all! |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Jun 23, 2021
…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. |
kibanamachine
added a commit
that referenced
this pull request
Jun 23, 2021
…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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
itemsarray) 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
shouldShowActiveForSubroutesflag, 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
itemssubnav into ourgenerateNavLinkhelper, which then usesitemsto 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