From aa274500cf906b16137704a9400456771ea9d1ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Wed, 5 Oct 2022 11:46:27 +0200 Subject: [PATCH] TopNav: Fixes to page and plugin page handling (#56278) * Added labels * App page fixes * Switch to switch * Fixing sort position for Apps --- .../src/components/PluginPage.tsx | 2 + pkg/services/navtree/models.go | 6 +- pkg/services/navtree/navtreeimpl/applinks.go | 120 ++++++++++-------- .../navtree/navtreeimpl/applinks_test.go | 14 +- public/app/core/components/PageNew/Page.tsx | 25 ++-- .../core/components/PageNew/PluginPage.tsx | 4 +- public/app/features/plugins/utils.test.ts | 13 ++ public/app/features/plugins/utils.ts | 2 +- 8 files changed, 119 insertions(+), 67 deletions(-) diff --git a/packages/grafana-runtime/src/components/PluginPage.tsx b/packages/grafana-runtime/src/components/PluginPage.tsx index 601a0fb56af..23b0de448d6 100644 --- a/packages/grafana-runtime/src/components/PluginPage.tsx +++ b/packages/grafana-runtime/src/components/PluginPage.tsx @@ -3,6 +3,8 @@ import React from 'react'; import { NavModelItem, PageLayoutType } from '@grafana/data'; export interface PluginPageProps { + /** Shown under main heading */ + subTitle?: React.ReactNode; pageNav?: NavModelItem; children: React.ReactNode; layout?: PageLayoutType; diff --git a/pkg/services/navtree/models.go b/pkg/services/navtree/models.go index a1cd112313a..ad625c5d023 100644 --- a/pkg/services/navtree/models.go +++ b/pkg/services/navtree/models.go @@ -20,6 +20,9 @@ const ( WeightDataConnections WeightPlugin WeightConfig + WeightAlertsAndIncidents + WeightMonitoring + WeightApps WeightAdmin WeightProfile WeightHelp @@ -40,6 +43,7 @@ const ( NavIDAlerting = "alerting" NavIDMonitoring = "monitoring" NavIDReporting = "reports" + NavIDApps = "apps" ) type NavLink struct { @@ -112,7 +116,7 @@ func (root *NavTreeRoot) RemoveEmptySectionsAndApplyNewInformationArchitecture(t if serverAdminNode := root.FindById(NavIDAdmin); serverAdminNode != nil { serverAdminNode.Url = "/admin/settings" serverAdminNode.Text = "Server admin" - serverAdminNode.SortWeight = 10000 + serverAdminNode.SortWeight = 0 if orgAdminNode != nil { orgAdminNode.Children = append(orgAdminNode.Children, serverAdminNode) diff --git a/pkg/services/navtree/navtreeimpl/applinks.go b/pkg/services/navtree/navtreeimpl/applinks.go index 133c5aa359e..d20b8b6cdc9 100644 --- a/pkg/services/navtree/navtreeimpl/applinks.go +++ b/pkg/services/navtree/navtreeimpl/applinks.go @@ -57,20 +57,8 @@ func (s *ServiceImpl) addAppLinks(treeRoot *navtree.NavTreeRoot, c *models.ReqCo }) } - if topNavEnabled { - treeRoot.AddSection(&navtree.NavLink{ - Text: "Apps", - Icon: "apps", - SubTitle: "App plugins that extend the Grafana experience", - Id: "apps", - Children: appLinks, - Section: navtree.NavSectionCore, - Url: s.cfg.AppSubURL + "/apps", - }) - } else { - for _, appLink := range appLinks { - treeRoot.AddSection(appLink) - } + for _, appLink := range appLinks { + treeRoot.AddSection(appLink) } return nil @@ -134,47 +122,79 @@ func (s *ServiceImpl) processAppPlugin(plugin plugins.PluginDTO, c *models.ReqCo } } - if len(appLink.Children) > 0 { - // If we only have one child and it's the app default nav then remove it from children - if len(appLink.Children) == 1 && appLink.Children[0].Url == appLink.Url { - appLink.Children = []*navtree.NavLink{} + // Apps without any nav children are not part of navtree + if len(appLink.Children) == 0 { + return nil + } + // If we only have one child and it's the app default nav then remove it from children + if len(appLink.Children) == 1 && appLink.Children[0].Url == appLink.Url { + appLink.Children = []*navtree.NavLink{} + } + + if !topNavEnabled { + return appLink + } + + // Remove default nav child + childrenWithoutDefault := []*navtree.NavLink{} + for _, child := range appLink.Children { + if child.Url != appLink.Url { + childrenWithoutDefault = append(childrenWithoutDefault, child) } + } + appLink.Children = childrenWithoutDefault - alertingNode := treeRoot.FindById(navtree.NavIDAlerting) + // Handle moving apps into specific navtree sections + alertingNode := treeRoot.FindById(navtree.NavIDAlerting) + sectionID := "apps" - if navConfig, hasOverride := s.navigationAppConfig[plugin.ID]; hasOverride && topNavEnabled { - appLink.SortWeight = navConfig.SortWeight + if navConfig, hasOverride := s.navigationAppConfig[plugin.ID]; hasOverride { + appLink.SortWeight = navConfig.SortWeight + sectionID = navConfig.SectionID + } - if navNode := treeRoot.FindById(navConfig.SectionID); navNode != nil { - navNode.Children = append(navNode.Children, appLink) - } else { - if navConfig.SectionID == navtree.NavIDMonitoring { - treeRoot.AddSection(&navtree.NavLink{ - Text: "Monitoring", - Id: navtree.NavIDMonitoring, - SubTitle: "Monitoring and infrastructure apps", - Icon: "heart-rate", - Section: navtree.NavSectionCore, - Children: []*navtree.NavLink{appLink}, - Url: s.cfg.AppSubURL + "/monitoring", - }) - } else if navConfig.SectionID == navtree.NavIDAlertsAndIncidents && alertingNode != nil { - treeRoot.AddSection(&navtree.NavLink{ - Text: "Alerts & incidents", - Id: navtree.NavIDAlertsAndIncidents, - SubTitle: "Alerting and incident management apps", - Icon: "bell", - Section: navtree.NavSectionCore, - Children: []*navtree.NavLink{alertingNode, appLink}, - Url: s.cfg.AppSubURL + "/alerts-and-incidents", - }) - treeRoot.RemoveSection(alertingNode) - } else { - s.log.Error("Plugin app nav id not found", "pluginId", plugin.ID, "navId", navConfig.SectionID) - } + if navNode := treeRoot.FindById(sectionID); navNode != nil { + navNode.Children = append(navNode.Children, appLink) + } else { + switch sectionID { + case navtree.NavIDApps: + treeRoot.AddSection(&navtree.NavLink{ + Text: "Apps", + Icon: "apps", + SubTitle: "App plugins that extend the Grafana experience", + Id: navtree.NavIDApps, + Children: []*navtree.NavLink{appLink}, + Section: navtree.NavSectionCore, + SortWeight: navtree.WeightApps, + Url: s.cfg.AppSubURL + "/apps", + }) + case navtree.NavIDMonitoring: + treeRoot.AddSection(&navtree.NavLink{ + Text: "Monitoring", + Id: navtree.NavIDMonitoring, + SubTitle: "Monitoring and infrastructure apps", + Icon: "heart-rate", + Section: navtree.NavSectionCore, + SortWeight: navtree.WeightMonitoring, + Children: []*navtree.NavLink{appLink}, + Url: s.cfg.AppSubURL + "/monitoring", + }) + case navtree.NavIDAlertsAndIncidents: + if alertingNode != nil { + treeRoot.AddSection(&navtree.NavLink{ + Text: "Alerts & incidents", + Id: navtree.NavIDAlertsAndIncidents, + SubTitle: "Alerting and incident management apps", + Icon: "bell", + Section: navtree.NavSectionCore, + SortWeight: navtree.WeightAlertsAndIncidents, + Children: []*navtree.NavLink{alertingNode, appLink}, + Url: s.cfg.AppSubURL + "/alerts-and-incidents", + }) + treeRoot.RemoveSection(alertingNode) } - } else { - return appLink + default: + s.log.Error("Plugin app nav id not found", "pluginId", plugin.ID, "navId", sectionID) } } diff --git a/pkg/services/navtree/navtreeimpl/applinks_test.go b/pkg/services/navtree/navtreeimpl/applinks_test.go index 96819370a9f..2ce70e01d5f 100644 --- a/pkg/services/navtree/navtreeimpl/applinks_test.go +++ b/pkg/services/navtree/navtreeimpl/applinks_test.go @@ -32,14 +32,14 @@ func TestAddAppLinks(t *testing.T) { Type: plugins.App, Includes: []*plugins.Includes{ { - Name: "Hello", + Name: "Catalog", Path: "/a/test-app1/catalog", Type: "page", AddToNav: true, DefaultNav: true, }, { - Name: "Hello", + Name: "Page2", Path: "/a/test-app1/page2", Type: "page", AddToNav: true, @@ -99,6 +99,16 @@ func TestAddAppLinks(t *testing.T) { require.Equal(t, "Test app1 name", treeRoot.Children[0].Children[0].Text) }) + t.Run("Should remove add default nav child when topnav is enabled", func(t *testing.T) { + service.features = featuremgmt.WithFeatures(featuremgmt.FlagTopnav) + treeRoot := navtree.NavTreeRoot{} + err := service.addAppLinks(&treeRoot, reqCtx) + require.NoError(t, err) + require.Equal(t, "Apps", treeRoot.Children[0].Text) + require.Equal(t, "Test app1 name", treeRoot.Children[0].Children[0].Text) + require.Equal(t, "Page2", treeRoot.Children[0].Children[0].Children[0].Text) + }) + t.Run("Should move apps that have specific nav id configured to correct section", func(t *testing.T) { service.features = featuremgmt.WithFeatures(featuremgmt.FlagTopnav) service.navigationAppConfig = map[string]NavigationAppConfig{ diff --git a/public/app/core/components/PageNew/Page.tsx b/public/app/core/components/PageNew/Page.tsx index 1b1b82875be..99dcb3de1c5 100644 --- a/public/app/core/components/PageNew/Page.tsx +++ b/public/app/core/components/PageNew/Page.tsx @@ -96,7 +96,7 @@ export const Page: PageType = ({ )} {layout === PageLayoutType.Canvas && ( -
+
{toolbar} {children}
@@ -137,14 +137,16 @@ const getStyles = (theme: GrafanaTheme2) => { top: theme.spacing(2), }, }), - wrapper: css` - height: 100%; - display: flex; - flex: 1 1 0; - flex-direction: column; - min-height: 0; - `, + wrapper: css({ + label: 'page-wrapper', + height: '100%', + display: 'flex', + flex: '1 1 0', + flexDirection: 'column', + minHeight: 0, + }), panes: css({ + label: 'page-panes', display: 'flex', height: '100%', width: '100%', @@ -156,18 +158,19 @@ const getStyles = (theme: GrafanaTheme2) => { }, }), pageContent: css({ + label: 'page-content', flexGrow: 1, }), pageInner: css({ + label: 'page-inner', padding: theme.spacing(3), boxShadow: shadow, background: theme.colors.background.primary, margin: theme.spacing(2, 2, 2, 1), - display: 'flex', - flexDirection: 'column', flexGrow: 1, }), - dashboardContent: css({ + canvasContent: css({ + label: 'canvas-content', display: 'flex', flexDirection: 'column', padding: theme.spacing(2), diff --git a/public/app/core/components/PageNew/PluginPage.tsx b/public/app/core/components/PageNew/PluginPage.tsx index f620545e9ae..41189849d1d 100644 --- a/public/app/core/components/PageNew/PluginPage.tsx +++ b/public/app/core/components/PageNew/PluginPage.tsx @@ -5,11 +5,11 @@ import { PluginPageContext } from 'app/features/plugins/components/PluginPageCon import { Page } from '../Page/Page'; -export function PluginPage({ children, pageNav, layout }: PluginPageProps) { +export function PluginPage({ children, pageNav, layout, subTitle }: PluginPageProps) { const context = useContext(PluginPageContext); return ( - + {children} ); diff --git a/public/app/features/plugins/utils.test.ts b/public/app/features/plugins/utils.test.ts index e034ea17d2b..6133741d3dc 100644 --- a/public/app/features/plugins/utils.test.ts +++ b/public/app/features/plugins/utils.test.ts @@ -10,6 +10,7 @@ describe('buildPluginSectionNav', () => { const app1: NavModelItem = { text: 'App1', id: 'plugin-page-app1', + url: '/a/plugin1', children: [ { text: 'page1', @@ -74,6 +75,18 @@ describe('buildPluginSectionNav', () => { expect(result?.node.text).toBe('page2'); }); + it('Should set app section to active', () => { + config.featureToggles.topnav = true; + const result = buildPluginSectionNav( + { pathname: '/a/plugin1', search: '' } as HistoryLocation, + null, + navIndex, + 'app1' + ); + expect(result?.main.children![0].active).toBe(true); + expect(result?.node.text).toBe('App1'); + }); + it('Should handle standalone page', () => { config.featureToggles.topnav = true; const result = buildPluginSectionNav( diff --git a/public/app/features/plugins/utils.ts b/public/app/features/plugins/utils.ts index b63ff766469..1bc6d90e292 100644 --- a/public/app/features/plugins/utils.ts +++ b/public/app/features/plugins/utils.ts @@ -76,7 +76,7 @@ export function buildPluginSectionNav( section.children = (section?.children ?? []).map((child) => { if (child.children) { return { - ...child, + ...setPageToActive(child, currentUrl), children: child.children.map((pluginPage) => setPageToActive(pluginPage, currentUrl)), }; }