-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[docs] High-level Routing, Navigation and URL overview #76888
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
Pinging @elastic/kibana-platform (Team:Platform) |
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.
Overall this is a great addition to the documentation.
Some comments, mostly on the fact that the basePath
handling seems to have been forgotten in the examples.
Thanks @pgayvallet, I made edits with your suggestions 👍 |
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.
Thanks for the changes. LGTM
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
|
||
[source,js] | ||
---- | ||
window.location.href = core.http.basePath.prepend(`/dashboard/my-dashboard`); // (try to avoid this) |
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.
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.
Great example, but I was initially confused because you say "using native browser apis" but part of the example is using core apis.
What if we had the "Deep linking into Kibana Apps" section first, so the user is introduced to the core api core.http.basePath.prepend
and what it does and that it should only be used if there isn't a url generator available?
Then you can follow up with this section and I think the user may understand that you are focusing on the window.location.href
part of the line.
Could then even write it as:
const discoverUrl = discoverUrlGenerator.createUrl({filters, timeRange});
// Avoid this, as it will cause a full page reload.
window.location.href = discoverUrl;
Which focuses in on which is the part to avoid.
---- | ||
|
||
|
||
As it would be too much boilerplate to do this for each {kib} link in your app, there is a handy HOC that helps with it: |
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.
Maybe we should not mention RedirectAppLinks
in this doc as it is a hack to easily make legacy Kibana URLs work. Or mention that it is a hack and you should not be using it.
It seems we are missing a good way to navigate across Kibana apps, we need a component where one specifies an app and path:
<KbnLink app={'discover'} to={'/foo/bar'}>
Click me!
</KbnLink>
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.
Maybe in kibana_react
we could put a component that can do both, navigate within Kibana app:
import { Link } from 'src/plugins/kibana_react/public';
<Link to={'/foo/bar'} />
and across apps if app
prop is specified:
<Link app={'discover'} to={'/foo/bar'} />
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 agree that component would makes sense.
RedirectAppLinks
still worth a mention. Some places don't have React wired up and you need to make sure your non React links are nested into that component.
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.
Maybe we should not mention RedirectAppLinks in this doc as it is a hack to easily make legacy Kibana URLs work. Or mention that it is a hack and you should not be using it.
Late reply, but RedirectAppLinks
is, AFAIK, not a hack, and the current preferred approach. See #58751 for more context and the whole discussion about the various options. The issue is still open for proposals for possible alternative to match other uses cases (the KbnLink
was one of them, so I'm totally in favor of adding that though, see #58751 (comment))
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.
So much ❤️ for these doc additions!! Thanks @Dosant!
Few nitpicky comments, but overall, LGTM!
|
||
[source,js] | ||
---- | ||
window.location.href = core.http.basePath.prepend(`/dashboard/my-dashboard`); // (try to avoid this) |
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.
Great example, but I was initially confused because you say "using native browser apis" but part of the example is using core apis.
What if we had the "Deep linking into Kibana Apps" section first, so the user is introduced to the core api core.http.basePath.prepend
and what it does and that it should only be used if there isn't a url generator available?
Then you can follow up with this section and I think the user may understand that you are focusing on the window.location.href
part of the line.
Could then even write it as:
const discoverUrl = discoverUrlGenerator.createUrl({filters, timeRange});
// Avoid this, as it will cause a full page reload.
window.location.href = discoverUrl;
Which focuses in on which is the part to avoid.
**When you should consider using state syncing utils:** | ||
|
||
* You want to sync your application state with URL in similar manner Analyze group applications do. | ||
* You want to follow platform's <<history-and-location, working with browser history and location best practices>> out of the box. |
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 don't follow this one. I don't see State Syncing utils, or the _a and _g format used in any of the documentation links for ScopedHistory:
- https://github.com/elastic/kibana/blob/master/docs/development/core/public/kibana-plugin-core-public.appmountparameters.history.md
- https://github.com/elastic/kibana/blob/master/test/plugin_functional/plugins/core_plugin_a/public/application.tsx#L120
- https://github.com/elastic/kibana/blob/master/docs/development/core/public/kibana-plugin-core-public.scopedhistory.md
So seems like I can use ScopedHistory without using any state syncing utils directly? Or am I missing something?
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.
So seems like I can use ScopedHistory without using any state syncing utils directly?
Exactly!
ScopedHistory
is a core's wrapper around browser's navigation and history apis.
- Whenever you want to use
window.location
orwindow.history
, you should consider using core'sScopeHistory
instead. (for example, reading / writing query params, updating path, etc..) - You can use it for listening to location changes. You can't do it with native apis. (Native api's are limited and support only popState or hashChange)
- You should initialize your app's react router with it.
- And you should initialize state state syncing utils with it! So inside state syncing utils are using core's history instance, instead of browser's api's directly
|
||
- You want to sync your application state with URL in similar manner analyze applications do that. | ||
- You want to follow platform's <<history-and-location, working with browser history and location best practices>> out of the box. | ||
- You want to support `state:storeInSessionStore` escape hatch for URL overflowing out of the box. |
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.
So state:storeInSessionStore
relies on _a and _g parameters in the URL?
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.
No, not really.
When using state syncing utils (with kbnUrlStateStorage
to be precise) you can configure your state to be synced to any query key. You can also set it up to sync only hash into URL. This is covered in docs of stateSync
, which is linked from this general guide: https://github.com/elastic/kibana/blob/master/src/plugins/kibana_utils/docs/state_sync/storages/kbn_url_storage.md#setting-url-format-option
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.
Thanks so much for writing this up! I read through the preview, but wasn't as thorough as I could have been. I did find one formatting error.
💚 Build SucceededBuild metricsdistributable file count
History
To update your PR or re-run it, just comment with: |
Summary
Closes #76288
This pr adds a page that gives a high-level overview of available tools and explains common approaches for handling routing, browser navigation and URL in Kibana.
Such guide is something we were asked for in one of the latest surveys