[Enterprise Search] Added a shouldShowActiveForSubroutes option #83338
Conversation
JasonStoltz
added
v7.11.0
release_note:skip
|
Ohh super good catch on this one!
I like this idea a lot in theory, but we might want to check with @scottybollinger / the WS team first. I think it would affect their current nested navigation links like so [current state below]:
would end up looking like this or this:
which is maybe a little odd, so I think I'm leaning towards a one-off prop like you added for that reason. |
There was a problem hiding this comment.
After reading the code a little more I think I feel more certain that we should leave the optional flag as-is instead of making it a default (I think mainly to not affect WS like the screenshots above). This is super great, thanks Jason! I have a super minor comment on test organization but it's non-blocking
| @@ -79,6 +79,30 @@ describe('SideNavLink', () => { | |||
| expect(wrapper.find('.enterpriseSearchNavLinks__item--isActive')).toHaveLength(1); | |||
| }); | |||
|
|
|||
| it("won't set an active class when route is a subroute of 'to'", () => { | |||
There was a problem hiding this comment.
[not a blocker] What are your thoughts on wrapping these two new tests in a describe('shouldShowActiveForSubroutes' () => { block for organization and swapping the order (testing the positive first and then confirming the negative)? no worries if not, it's super minor
There was a problem hiding this comment.
Yeah I'll update this. I have to merge master anyway.
| @@ -63,15 +63,17 @@ export const SideNav: React.FC<SideNavProps> = ({ product, children }) => { | |||
|
|
|||
| interface SideNavLinkProps { | |||
| to: string; | |||
| shouldShowActiveForSubroutes?: boolean; | |||
There was a problem hiding this comment.
After rereading a few times this var name is really growing on me! It's very clear what it means and I was able to figure it out without having to ask for a screenshot (which I almost did at first haha) 💯
There was a problem hiding this comment.
Haha. I wanted to, but it was late in the day and and I was trying to get out the door so I thought I'd roll the dice 🎲
|
Just to be clear, I believe we want Workplace Search as-is with the parent not active. That won't change as a result of this PR, right? |
|
Correct! The current implementation is with a manual flag/prop for if you want that behavior |
JasonStoltz
changed the title
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…ode-details * 'master' of github.com:elastic/kibana: Remove dependency of tests on strict SyntaxKind values (elastic#83440) [SecuritySolution] override timerange for prebuilt templates (elastic#82468) [Enterprise Search] Added a shouldShowActiveForSubroutes option (elastic#83338) [Lens] Make the dimension flyout panel stay close on outside click (elastic#83059) [Security Solution] Gracefully handle errors in detection rules install (elastic#83306) Fix advanced settings category sorting (elastic#83394)
Summary
This PR lets us show Engine nav links as "active" for routes that have sub-routes.
For ex.
There is a documents page at
/documents.There is a document detail page at
/documents/:id.The documents page is linked to from the Engine nav.
The document detail page is considered a sub-page of documents, and hence, is not linked from the Engine nav.
However, we still need to show "Documents" as the active link within the Engine nav.
I'm proposing passing a flag of
shouldShowActiveForSubroutesto theSubNavLink, which will show the link as active if the "to" link matches the current path OR is the root of the current path.Alternatively, we could make this change the default for nav links. I.e., if the "to" link matches the current path OR is the root of the current path, then show it as active.
I'm open to suggestions.
Checklist
For maintainers