Skip to content
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

Add application deep links to global search #83380

Merged
merged 7 commits into from
Nov 30, 2020

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Nov 13, 2020

Summary

Lays the foundation for solving #72680

This PR introduces a new concept to Core's ApplicationService called subLinks. Applications may register subLinks with Core to represent secondary navigation locations within a single app. These subLinks are then leveraged by global search to provide navigation deep into applications.

In this initial PR, I have integrated the Management app sections with this feature. Once this lands, I will work with individual teams to add their sublinks to global search.

image

Release Notes

Kibana's navigation search now includes deep links into various applications, allowing you to quickly navigate directly to the screens you need most.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc enhancement New value added to drive a business result v8.0.0 v7.11.0 labels Nov 13, 2020
@ryankeairns
Copy link
Contributor

@joshdover this is AWESOME

@ryankeairns
Copy link
Contributor

I'm not sure if this is a me thing, but I received this type error when trying to start up this PR locally (perhaps I am jumping the gun, apologies if so):

ERROR Error: Command failed with exit code 2: /Users/elastic/projects/kibana/node_modules/typescript/bin/tsc -b tsconfig.refs.json --pretty
            src/core/public/chrome/nav_links/to_nav_link.test.ts:25:68 - error TS2322: Type '{ id: string; status: AppStatus; navLinkStatus: AppNavLinkStatus; tooltip?: string | undefined; defaultPath?: string | undefined; title: string; category?: AppCategory | undefined; ... 7 more ...; subLinks?: PublicAppSubLinkInfo[] | undefined; }' is not assignable to type 'PublicAppInfo'.
              Type '{ id: string; status: AppStatus; navLinkStatus: AppNavLinkStatus; tooltip?: string | undefined; defaultPath?: string | undefined; title: string; category?: AppCategory | undefined; ... 7 more ...; subLinks?: PublicAppSubLinkInfo[] | undefined; }' is not assignable to type '{ status: AppStatus; navLinkStatus: AppNavLinkStatus; appRoute: string; subLinks: PublicAppSubLinkInfo[]; }'.
                Types of property 'subLinks' are incompatible.
                  Type 'PublicAppSubLinkInfo[] | undefined' is not assignable to type 'PublicAppSubLinkInfo[]'.
                    Type 'undefined' is not assignable to type 'PublicAppSubLinkInfo[]'.

             25 const app = (props: Partial<PublicAppInfo> = {}): PublicAppInfo => ({
                                                                                   ~~
             26   id: 'some-id',
                ~~~~~~~~~~~~~~~~
            ...
             31   ...props,
                ~~~~~~~~~~~
             32 });
                ~~


            Found 1 error.

@ryankeairns
Copy link
Contributor

Regarding the left nav issue you noted above... there is a current effort to establish an (optional) side nav pattern for solutions. Both of these - sublinks in search and an 'always visible' side nav - should allow us to avoid the mounting situation you describe above.

@joshdover
Copy link
Contributor Author

@ryankeairns Yep I missed a type error 🤦

Glad this is going in the right direction. What do we think about how this is displayed? Are we ok with just using this "slash" pattern?

@ryankeairns
Copy link
Contributor

Thanks for fixing the error, pulled it up and it works as expected.

Generally speaking, the slash pattern works, although I think the first part/provider (Stack Management) would make sense as the metadata subtitle (i.e. change from Management to Stack Management).

That would follow existing patterns elsewhere, for example how Uptime has metadata of Observability.
In this case, Data / Index Management would have metadata of Stack Management.

Screen Shot 2020-11-16 at 9 54 57 PM


Side note: this particular change will likely bring attention to the fact that solution providers themselves do not appear in search results. It's not a problem to be addressed here, I'll just point it out as it will likely come up when somebody searches security and see all the Stack Management results and no Security (the solution) results.

Screen Shot 2020-11-16 at 10 05 08 PM

We probably need to have them register their solution name as a keyword that directs to their Overview page, perhaps... but I digress :)

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I'm good with the chosen approach, I just got one question:

In the future, I would also like to leverage the subLinks concept to solve a problem that the Security Solution app currently faces. What they would like to do is have multiple navigation links in the main sidebar into the same application

Even if I can only agree that we need/want subLink in nav support, I'm wondering if we aren't mixing things up here. For example, for the management section sublinks, we will never want to display them in the navbar. Adding a status / navLinkStatus to the sublinks will also not be enough, as the application result provider current behavior is to exclude hidden apps / links from its results. If we go that way, we will probably need to add a includeInGlobalSearch (or similar) flag to apps/sublinks.

Comment on lines 254 to 255
| { path: string; subLinks?: AppSubLink[] }
| { path?: string; subLinks: AppSubLink[] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be cases where a sublink can have both an 'existence' ( a path) and 'children' (subLinks)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could in theory I think, yes. For instance, there may be an overview page for that section of the hierarchy, but also have sublinks inside of it. I typed it this way to make it clear that it has to have at least one of path or subLinks.

Comment on lines 84 to 74
id: subLink ? `${app.id}-${subLink.id}` : app.id,
title: subLink?.title ?? app.title,
type: 'application',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want these sublinks to be of the application type, or do we want a new type, such as feature or something? Keeping application is easier to integrate with the searchbar's per-type logic, but I'm wondering if we may want to dissociate these results from the base/current application ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it felt like it made sense to have the scoring for these done in a single place so that the more specific sublinks are ranked higher than the general applications when possible. Happy to experiment with a separate provider if we feel that is better.

@joshdover
Copy link
Contributor Author

Even if I can only agree that we need/want subLink in nav support, I'm wondering if we aren't mixing things up here. For example, for the management section sublinks, we will never want to display them in the navbar. Adding a status / navLinkStatus to the sublinks will also not be enough, as the application result provider current behavior is to exclude hidden apps / links from its results. If we go that way, we will probably need to add a includeInGlobalSearch (or similar) flag to apps/sublinks.

All good points. Also with an eye towards the new navigation UX patterns that are being experimented with, we may not need this at all. In that case, I'll simply design this with the one known use case in mind.

@joshdover
Copy link
Contributor Author

Generally speaking, the slash pattern works, although I think the first part/provider (Stack Management) would make sense as the metadata subtitle (i.e. change from Management to Stack Management).

I'm not sure exactly how we should handle this. The 'Stack Management' application currently belongs to the 'Management' category which also contains Fleet, Dev Tools, and Stack Management. The simplest solution would be to remove the Stack Management / prefix from all of the management links, but it sort of breaks the hierarchy right now of Category -> Application -> Sublink.

I'll throw up a change here a bit with that in place and we can see how it feels.

@joshdover
Copy link
Contributor Author

Generally speaking, the slash pattern works, although I think the first part/provider (Stack Management) would make sense as the metadata subtitle (i.e. change from Management to Stack Management).

I'm not sure exactly how we should handle this. The 'Stack Management' application currently belongs to the 'Management' category which also contains Fleet, Dev Tools, and Stack Management. The simplest solution would be to remove the Stack Management / prefix from all of the management links, but it sort of breaks the hierarchy right now of Category -> Application -> Sublink.

I'll throw up a change here a bit with that in place and we can see how it feels.

This also creates a situation where the default results (alphabetized list of app links) does not group the Stack Management links together. I'm not sure if this is desired:

image

@ryankeairns
Copy link
Contributor

This also creates a situation where the default results (alphabetized list of app links) does not group the Stack Management links together. I'm not sure if this is desired:

Yeah, that's not great.

How are the sub links scored? It may not be possible, but I'm wondering if sub links should be scored lower than applications (that appear in the main left nav). So, only Dev Tools, Stack Management, and Fleet would appear in the initial set of results (apps sorted alphabetically), while the sub links would not.

Once you enter a search term, then the sub links would appear after app results. I think I just spent a lot of words to say - can we score sub links lower than apps but above SOs :D

@joshdover joshdover force-pushed the app-sublinks branch 2 times, most recently from 1556632 to 9395b6c Compare November 24, 2020 00:12
@joshdover
Copy link
Contributor Author

Once you enter a search term, then the sub links would appear after app results. I think I just spent a lot of words to say - can we score sub links lower than apps but above SOs :D

Done 😄. Sublinks will not show in the default results now, but are included as soon as you start typing.

@ryankeairns
Copy link
Contributor

YASSSSS that did the trick, I love it! Thanks

These results are what I was hoping to see (in addition to only showing top level apps in the initial set):

Screen Shot 2020-11-24 at 8 43 51 AM

Screen Shot 2020-11-24 at 8 43 41 AM

Screen Shot 2020-11-24 at 8 43 32 AM

Screen Shot 2020-11-24 at 8 43 18 AM

@joshdover joshdover marked this pull request as ready for review November 24, 2020 14:53
@joshdover joshdover requested a review from a team November 24, 2020 14:53
@joshdover joshdover requested a review from a team as a code owner November 24, 2020 14:53
@joshdover joshdover requested a review from a team November 24, 2020 14:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@joshdover
Copy link
Contributor Author

joshdover commented Nov 25, 2020

My main concerns is regarding applications that are hidden, but still need to register some 'sub' links. As we are still automatically excluding the hidden apps, it is not currently possible.

We already have a case of such need: Lens. The app is currently registering its own result provider to (only) display the lens app result. With current implementation, we cannot leverage this new feature to get rid if it.

What I was thinking here is that the Visualize plugin/app could use the appUpdater$ API for adding subLinks for any registered visualization types as actions. But, yes we should discuss what the desired UX here is. With this pattern, Visualize could add links like Visualize / Create Lens. Under-the-hood this may end up being a redirect to a different application, but it allows the Visualize app to be in control of and add consistent Create <X> subLinks that will be logically grouped in a way that users would most likely expect.

If we need to have custom titles or euiIconTypes for subLinks I can certainly add that feature. I think it all just depends on what we want the end-result UX to be.

Last thing, depending on #83380 (comment), if we know this is going to only be used for GS/navSearch results (which is ihmo totally fine and way more pragmatic than trying to mix chrome nav links and search results), we may want to rename App.subLinks to something more explicitly related to the navSearch.

+1 on this. How about searchDeepLinks?

@ryankeairns
Copy link
Contributor

Also important to note that the Lens result should also be displayed on the default result display, which is not possible atm as we are only displaying 'root' level app results (but this makes me wonder if Lens should then really be considered as a 'feature' of visualize). Maybe we just want to be able to display a app-level result for Lens even if the app is hidden, in which case this would be a totally different feature than what this PR is addressing

Related, I've put a question out to the Lens team to see whether or not there are plans for Lens to appear as a distinct app in the left navigation menu.

@pgayvallet
Copy link
Contributor

Overall, I'm good merging the PR in its current state.

How about searchDeepLinks

Sounds good to me.

If we need to have custom titles or euiIconTypes for subLinks I can certainly add that feature. I think it all just depends on what we want the end-result UX to be.

Yea. that can easily be added later though, so I'm fine waiting until the need arises.

With this pattern, Visualize could add links like Visualize / Create Lens. Under-the-hood this may end up being a redirect to a different application, but it allows the Visualize app to be in control of and add consistent Create subLinks that will be logically grouped in a way that users would most likely expect.

I think that would make sense, as long as design/product is fine with it.

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment.

@ryankeairns
Copy link
Contributor

Also important to note that the Lens result should also be displayed on the default result display, which is not possible atm as we are only displaying 'root' level app results (but this makes me wonder if Lens should then really be considered as a 'feature' of visualize). Maybe we just want to be able to display a app-level result for Lens even if the app is hidden, in which case this would be a totally different feature than what this PR is addressing

Related, I've put a question out to the Lens team to see whether or not there are plans for Lens to appear as a distinct app in the left navigation menu.

There are no current plans to display Lens in the left nav, however this question did stir up some existential questions around how far we lean into the dashboard-first approach and the resulting impacts to the discoverability of Lens. I'll spin this off into another issue for further discussion.

@joshdover joshdover changed the title Add application sublinks to global search Add application deep links to global search Nov 30, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 547.1KB 547.7KB +526.0B
globalSearchBar 33.2KB 33.2KB +46.0B
globalSearchProviders 6.8KB 8.8KB +2.0KB
management 22.0KB 22.2KB +269.0B
total +2.8KB

History

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

@ryankeairns
Copy link
Contributor

The issue that I spun off for Lens has more to do with product-level planning than this feature, in particular. I'm hopeful we can still get this merged and continue the Lens discussion separately.

@joshdover joshdover merged commit d93e211 into elastic:master Nov 30, 2020
@joshdover joshdover deleted the app-sublinks branch November 30, 2020 22:54
joshdover added a commit to joshdover/kibana that referenced this pull request Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result release_note:enhancement Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants