-
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
[Ingest Manager] Consolidate routing and add breadcrumbs to all pages #66475
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
return core.http.basePath.prepend(`${BASE_PATH}#${path}`); | ||
} | ||
return { | ||
getPath: (page: StaticPage | DynamicPage, values?: DynamicPagePathValues) => { |
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.
why not returning getPath
directly? we can avoid to create a new function
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.
good catch, leftover logic from when I DRY'd getPath
😅
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 checked this out locally and clicked around. The breadcrumb changes LGTM and are reason enough for me to ship.
I'd like to revisit the links later but anything I'd want to change is also much easier thanks to this PR. It's a big improvement over what we have now. Thanks!
edit_datasource: ({ configId, datasourceId }) => | ||
`/configs/${configId}/edit-datasource/${datasourceId}`, | ||
fleet: () => '/fleet', | ||
fleet_agent_list: () => '/fleet/agents', |
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 saw the kuery
param used somewhere. Can we change to something like
fleet_agent_list: () => '/fleet/agents', | |
fleet_agent_list: ({ kuery }) => `/fleet/agents?kuery=${kuery}`', |
<EuiLink | ||
href={`${getHref( | ||
'fleet_agent_list' | ||
)}?kuery=${AGENT_SAVED_OBJECT_TYPE}.config_id : ${agentConfigId}`} |
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.
Can this go into an options object?
panel: 'settings', | ||
}); | ||
window.location.href = settingsUrl; | ||
history.push(settingsPath); |
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.
🙌
Hi @jen-huang my only suggestion would be for the Plugin unmount callback to reset the breadcrumb. I recently noticed that the breadcrumb seems to persist across Plugins - noticed it in Endpoint where we are not currently setting it and were seeing other plugin's crumbs :) |
… and teardown breadcrumb and doc title
Thanks for the reviews everyone. I pushed up a change that addresses the concerns 🙂 |
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…elastic#66475) * Initial pass at normalizing links and redirects * Remove unused nav components * Normalize & centralize routing paths * Add breadcrumbs to all pages * Remove unused var * Change /epm to /integrations (elastic#66030) * Fixes from review: kuery param, getPath cleanup, use chrome.docTitle, and teardown breadcrumb and doc title Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…#66475) (#66668) * Initial pass at normalizing links and redirects * Remove unused nav components * Normalize & centralize routing paths * Add breadcrumbs to all pages * Remove unused var * Change /epm to /integrations (#66030) * Fixes from review: kuery param, getPath cleanup, use chrome.docTitle, and teardown breadcrumb and doc title Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…ent/add-support-in-url-for-hidden-toggle * 'master' of github.com:elastic/kibana: (34 commits) [SIEM][CASE] Fix bug when connector is deleted. (elastic#65876) [SIEM][CASE] Improve layout (elastic#66232) [Index Management] Support Hidden Indices (elastic#66422) Add Login Selector functional tests. (elastic#65705) Lens drilldowns (elastic#65675) [ML] Custom template for apiDoc markdown (elastic#66567) Don't bootstrap core type emits (elastic#66377) [Dashboard] Improve loading error handling (elastic#66372) [APM] Minor style fixes for the node strokes (elastic#66574) [Ingest Manager] Fix create data source from integration (elastic#66626) [Metrics UI] Fix default metric alert interval for new conditions (elastic#66610) [Metrics UI] Fix alignment and allow clearing metric value (elastic#66589) Don't return package name for non-package data streams (elastic#66606) [Ingest Manager] Consolidate routing and add breadcrumbs to all pages (elastic#66475) [Docs/Reporting] Have the docs about granular timeout match Cloud docs (elastic#66267) Don't automatically add license header to code inside plugins dir. (elastic#66601) [APM] Don't trigger map layout if no elements (elastic#66625) [Logs UI] Validate ML job setup time ranges (elastic#66426) Fix pagination bugs in CCR and Remote Clusters (elastic#65931) Add cloud icon for supported settings and embed single-sourced getting started (elastic#65610) ... # Conflicts: # x-pack/plugins/index_management/public/application/sections/home/index_list/index_table/index_table.js # x-pack/plugins/index_management/server/lib/fetch_indices.ts
Resolves #63981
Resolves #66030
Summary
This PR adds breadcrumbs to all pages. As part of this work, I also consolidated our routing and linking constants and methods (the breadcrumbs work would have been messy otherwise 🙂). More detailed summary of changes:
constants/page_paths.ts
was added which exportsPAGE_ROUTING_PATHS
andpagePathGetters
, as well as types for page namesPAGE_ROUTING_PATHS
are strings used for declaring React routes i.e.<Routh path={...}>
pagePathGetters
are methods to retrieve/build a path to a specific pagepagePathGetters
fromPAGE_ROUTING_PATHS
as it's trivial to update paths in both placesuseLink()
hook was updated to return two methods:getHref()
andgetPath()
useLink
more easily in componentsgetHref
is used for links that appear in the UI, it builds a full path that includes the Kibana base pathgetPath
does not include the Kibana base path and is used for React router'shistory.push
useLink()
useBreadcrumbs()
hook was created and implemented across all pagesuseBreadcrumbs()
also sets the page title by reversing breadcrumbsUpdating routes becomes trivial with these changes. For example in 7a09c91 I updated
/epm
routes to/integrations
to resolve #66030. This updated all links including breadcrumbs.Screenshots