From dd0c250033c881e64385e3815afb4d3e5a7ca545 Mon Sep 17 00:00:00 2001 From: Allison King Date: Tue, 18 Apr 2023 15:30:02 -0400 Subject: [PATCH 1/6] Add recursive side nav for children nav items --- .../src/features/common/nav/v2/NavSideBar.tsx | 36 ++++-- .../src/features/common/nav/v2/nav-config.ts | 114 +++++++++++++----- 2 files changed, 111 insertions(+), 39 deletions(-) diff --git a/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx b/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx index cc57fd18b97..7563f0723e7 100644 --- a/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx +++ b/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx @@ -1,11 +1,33 @@ import { NavList } from "@fidesui/components"; -import { Heading, VStack } from "@fidesui/react"; +import { Box, Heading, VStack } from "@fidesui/react"; import { useRouter } from "next/router"; import React from "react"; import { useNav } from "./hooks"; +import { NavGroupChild } from "./nav-config"; import { NavSideBarLink } from "./NavLink"; +const NavListItem = ({ + title, + path, + children, + level = 0, +}: NavGroupChild & { level?: number }) => { + const router = useRouter(); + const isActive = router.pathname.startsWith(path); + + return ( + + + {title} + + {children.map((childRoute) => ( + + ))} + + ); +}; + export const NavSideBar = () => { const router = useRouter(); const nav = useNav({ path: router.pathname }); @@ -19,15 +41,9 @@ export const NavSideBar = () => { {nav.active.title} - {nav.active.children.map(({ title, path }) => { - const isActive = router.pathname.startsWith(path); - - return ( - - {title} - - ); - })} + {nav.active.children.map((childRoute) => ( + + ))} ); diff --git a/clients/admin-ui/src/features/common/nav/v2/nav-config.ts b/clients/admin-ui/src/features/common/nav/v2/nav-config.ts index 0942bdae1b9..930ada24bc4 100644 --- a/clients/admin-ui/src/features/common/nav/v2/nav-config.ts +++ b/clients/admin-ui/src/features/common/nav/v2/nav-config.ts @@ -11,6 +11,8 @@ export type NavConfigRoute = { requiresFlag?: FlagNames; /** This route is only available if the user has ANY of these scopes */ scopes: ScopeRegistryEnum[]; + /** Child routes which will be rendered in the side nav */ + routes?: NavConfigRoute[]; }; export type NavConfigGroup = { @@ -50,11 +52,21 @@ export const NAV_CONFIG: NavConfigGroup[] = [ scopes: [ScopeRegistryEnum.MESSAGING_CREATE_OR_UPDATE], }, { - title: "Privacy notices", + title: "Consent", + // For now, we don't have a full Consent page, so just use the privacy notice route path: routes.PRIVACY_NOTICES_ROUTE, requiresFlag: "privacyNotices", requiresPlus: true, scopes: [ScopeRegistryEnum.PRIVACY_NOTICE_READ], + routes: [ + { + title: "Privacy notices", + path: routes.PRIVACY_NOTICES_ROUTE, + requiresFlag: "privacyNotices", + requiresPlus: true, + scopes: [ScopeRegistryEnum.PRIVACY_NOTICE_READ], + }, + ], }, ], }, @@ -147,6 +159,7 @@ export type NavGroupChild = { title: string; path: string; exact?: boolean; + children: Array; }; export type NavGroup = { @@ -203,17 +216,73 @@ const navRouteInScope = ( return true; }; +interface ConfigureNavProps { + config: NavConfigGroup[]; + userScopes: ScopeRegistryEnum[]; + hasPlus?: boolean; + flags?: Record; +} + +const configureNavRoute = ({ + route, + hasPlus, + flags, + userScopes, + navGroupTitle, +}: Omit & { + route: NavConfigRoute; + navGroupTitle: string; +}): NavGroupChild | undefined => { + // If the target route would require plus in a non-plus environment, + // exclude it from the group. + if (route.requiresPlus && !hasPlus) { + return undefined; + } + + // If the target route is protected by a feature flag that is not enabled, + // exclude it from the group + if (route.requiresFlag && (!flags || !flags[route.requiresFlag])) { + return undefined; + } + + // If the target route is protected by a scope that the user does not + // have, exclude it from the group + if (!navRouteInScope(route, userScopes)) { + return undefined; + } + + const children: NavGroupChild["children"] = []; + if (route.routes) { + route.routes.forEach((childRoute) => { + const configuredChildRoute = configureNavRoute({ + route: childRoute, + userScopes, + hasPlus, + flags, + navGroupTitle, + }); + if (configuredChildRoute) { + children.push(configuredChildRoute); + } + }); + } + + const groupChild: NavGroupChild = { + title: route.title ?? "", // ?? navGroup.title, + path: route.path, + exact: route.exact, + children, + }; + + return groupChild; +}; + export const configureNavGroups = ({ config, userScopes, hasPlus = false, flags, -}: { - config: NavConfigGroup[]; - userScopes: ScopeRegistryEnum[]; - hasPlus?: boolean; - flags?: Record; -}): NavGroup[] => { +}: ConfigureNavProps): NavGroup[] => { const navGroups: NavGroup[] = []; config.forEach((group) => { @@ -228,29 +297,16 @@ export const configureNavGroups = ({ navGroups.push(navGroup); group.routes.forEach((route) => { - // If the target route would require plus in a non-plus environment, - // exclude it from the group. - if (route.requiresPlus && !hasPlus) { - return; - } - - // If the target route is protected by a feature flag that is not enabled, - // exclude it from the group - if (route.requiresFlag && (!flags || !flags[route.requiresFlag])) { - return; - } - - // If the target route is protected by a scope that the user does not - // have, exclude it from the group - if (!navRouteInScope(route, userScopes)) { - return; - } - - navGroup.children.push({ - title: route.title ?? navGroup.title, - path: route.path, - exact: route.exact, + const routeConfig = configureNavRoute({ + route, + hasPlus, + flags, + userScopes, + navGroupTitle: group.title, }); + if (routeConfig) { + navGroup.children.push(routeConfig); + } }); }); From 557528271a201a28bd54888d3da4772aab44532b Mon Sep 17 00:00:00 2001 From: Allison King Date: Tue, 18 Apr 2023 16:45:49 -0400 Subject: [PATCH 2/6] Refactor NavSideBar so it is easier to test --- .../src/features/common/nav/v2/NavSideBar.tsx | 57 +++++++++++++------ .../src/features/common/nav/v2/nav-config.ts | 2 +- 2 files changed, 42 insertions(+), 17 deletions(-) diff --git a/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx b/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx index 7563f0723e7..f39d92cd9b1 100644 --- a/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx +++ b/clients/admin-ui/src/features/common/nav/v2/NavSideBar.tsx @@ -1,37 +1,51 @@ import { NavList } from "@fidesui/components"; -import { Box, Heading, VStack } from "@fidesui/react"; +import { Heading, UnorderedList, VStack } from "@fidesui/react"; import { useRouter } from "next/router"; import React from "react"; import { useNav } from "./hooks"; -import { NavGroupChild } from "./nav-config"; +import type { ActiveNav, NavGroup, NavGroupChild } from "./nav-config"; import { NavSideBarLink } from "./NavLink"; const NavListItem = ({ title, path, children, - level = 0, -}: NavGroupChild & { level?: number }) => { - const router = useRouter(); - const isActive = router.pathname.startsWith(path); + routerPathname, +}: NavGroupChild & { routerPathname: string }) => { + const isActive = routerPathname.startsWith(path); return ( - + <> {title} - {children.map((childRoute) => ( - - ))} - + {children.length ? ( + + {children.map((childRoute) => ( + + ))} + + ) : null} + ); }; -export const NavSideBar = () => { - const router = useRouter(); - const nav = useNav({ path: router.pathname }); - +/** + * Similar to NavSideBar, but without hooks so that it is easier to test + */ +export const UnconnectedNavSideBar = ({ + routerPathname, + ...nav +}: { + groups: NavGroup[]; + active: ActiveNav | undefined; + routerPathname: string; +}) => { // Don't render the sidebar if no group is active if (!nav.active) { return null; @@ -42,9 +56,20 @@ export const NavSideBar = () => { {nav.active.title} {nav.active.children.map((childRoute) => ( - + ))} ); }; + +export const NavSideBar = () => { + const router = useRouter(); + const nav = useNav({ path: router.pathname }); + + return ; +}; diff --git a/clients/admin-ui/src/features/common/nav/v2/nav-config.ts b/clients/admin-ui/src/features/common/nav/v2/nav-config.ts index 930ada24bc4..7510116a5cd 100644 --- a/clients/admin-ui/src/features/common/nav/v2/nav-config.ts +++ b/clients/admin-ui/src/features/common/nav/v2/nav-config.ts @@ -268,7 +268,7 @@ const configureNavRoute = ({ } const groupChild: NavGroupChild = { - title: route.title ?? "", // ?? navGroup.title, + title: route.title ?? navGroupTitle, path: route.path, exact: route.exact, children, From f8722668d5654c3a4cc90b6c6dbee2465a31382e Mon Sep 17 00:00:00 2001 From: Allison King Date: Tue, 18 Apr 2023 16:46:05 -0400 Subject: [PATCH 3/6] Add cypress component test for NavSideBar --- .../cypress/components/NavSideBar.cy.tsx | 126 ++++++++++++++++++ clients/admin-ui/tsconfig.json | 3 +- 2 files changed, 128 insertions(+), 1 deletion(-) create mode 100644 clients/admin-ui/cypress/components/NavSideBar.cy.tsx diff --git a/clients/admin-ui/cypress/components/NavSideBar.cy.tsx b/clients/admin-ui/cypress/components/NavSideBar.cy.tsx new file mode 100644 index 00000000000..d3d7dbbd2b2 --- /dev/null +++ b/clients/admin-ui/cypress/components/NavSideBar.cy.tsx @@ -0,0 +1,126 @@ +import * as React from "react"; + +import { + configureNavGroups, + findActiveNav, + NavConfigGroup, +} from "~/features/common/nav/v2/nav-config"; +import { UnconnectedNavSideBar } from "~/features/common/nav/v2/NavSideBar"; + +const ACTIVE_COLOR = "rgb(130, 78, 242)"; +const INACTIVE_COLOR = "rgb(45, 55, 72)"; + +const selectLinkColor = (title: string) => + cy.contains("a", title).should("have.css", "color"); + +const verifyActiveState = ({ + active, + inactive, +}: { + active: string[]; + inactive: string[]; +}) => { + active.forEach((title) => { + selectLinkColor(title).should("eql", ACTIVE_COLOR); + }); + inactive.forEach((title) => { + selectLinkColor(title).should("eql", INACTIVE_COLOR); + }); +}; + +describe("NavSideBar", () => { + it("renders children nav links", () => { + const config: NavConfigGroup[] = [ + { + title: "Privacy requests", + routes: [ + { + path: "/privacy-requests", + scopes: [], + }, + { + title: "Consent", + path: "/consent", + scopes: [], + routes: [ + { + title: "Privacy notices", + path: "/consent/privacy-notices", + scopes: [], + routes: [ + { + title: "3rd level page", + path: "/consent/privacy-notices/third-level-page", + scopes: [], + }, + ], + }, + { + title: "Cookies", + path: "/consent/cookies", + scopes: [], + }, + ], + }, + ], + }, + ]; + const navGroups = configureNavGroups({ + config, + userScopes: [], + }); + const path = "/consent/privacy-notices"; + const activeNav = findActiveNav({ navGroups, path }); + + // First check if the active path is /consent/privacy-notices + cy.mount( + + ); + + cy.contains("nav li", "Privacy requests"); + cy.contains("nav li", "Consent").within(() => { + cy.contains("a", "Privacy notices"); + cy.contains("a", "Cookies"); + }); + + verifyActiveState({ + active: ["Privacy notices", "Consent"], + inactive: ["Privacy requests", "3rd level page", "Cookies"], + }); + + // Check if the active path is only on the first level + cy.mount( + + ); + verifyActiveState({ + active: ["Consent"], + inactive: [ + "Privacy notices", + "Privacy requests", + "3rd level page", + "Cookies", + ], + }); + + // Check if the active path is deeper nested + cy.mount( + + ); + verifyActiveState({ + active: ["Consent", "Privacy notices", "3rd level page"], + inactive: ["Privacy requests", "Cookies"], + }); + }); +}); diff --git a/clients/admin-ui/tsconfig.json b/clients/admin-ui/tsconfig.json index c4dfe320642..eac8a9da2eb 100644 --- a/clients/admin-ui/tsconfig.json +++ b/clients/admin-ui/tsconfig.json @@ -37,6 +37,7 @@ "exclude": [ "node_modules", "cypress/**/*.ts", - "../../admin-ui/cypress.config.ts" + "cypress/**/*.tsx", + "cypress.config.ts" ] } From 8c9b53a00c99818467ae9b2d3a6bd9a0c0d0924f Mon Sep 17 00:00:00 2001 From: Allison King Date: Tue, 18 Apr 2023 16:57:18 -0400 Subject: [PATCH 4/6] Restore component tests in CI --- .github/workflows/frontend_checks.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/workflows/frontend_checks.yml b/.github/workflows/frontend_checks.yml index b8c933f6036..005bb59933f 100644 --- a/.github/workflows/frontend_checks.yml +++ b/.github/workflows/frontend_checks.yml @@ -79,6 +79,13 @@ jobs: name: cypress-videos path: /home/runner/work/fides/fides/clients/admin-ui/cypress/videos/*.mp4 + - name: Cypress component tests + uses: cypress-io/github-action@v4 + with: + working-directory: clients/admin-ui + install: false + component: true + Privacy-Center-Unit: runs-on: ubuntu-latest strategy: From 2ce39d7552dbe7ec9f4679b786650e9523c4c912 Mon Sep 17 00:00:00 2001 From: Allison King Date: Tue, 18 Apr 2023 17:05:32 -0400 Subject: [PATCH 5/6] Remove unused exclusion --- clients/admin-ui/tsconfig.json | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/clients/admin-ui/tsconfig.json b/clients/admin-ui/tsconfig.json index eac8a9da2eb..e5893432549 100644 --- a/clients/admin-ui/tsconfig.json +++ b/clients/admin-ui/tsconfig.json @@ -34,10 +34,5 @@ "**/*.tsx", "**/*.d.ts" ], - "exclude": [ - "node_modules", - "cypress/**/*.ts", - "cypress/**/*.tsx", - "cypress.config.ts" - ] + "exclude": ["node_modules", "cypress/**/*.ts", "cypress/**/*.tsx"] } From d73bcf4073b63311696e7665db19bc41d448ef3d Mon Sep 17 00:00:00 2001 From: Allison King Date: Wed, 19 Apr 2023 11:49:43 -0400 Subject: [PATCH 6/6] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53067f043ba..4c2708ba9b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The types of changes are: - Access and erasure support for Aircall [#2589](https://github.com/ethyca/fides/pull/2589) - Access and erasure support for Klaviyo [#2501](https://github.com/ethyca/fides/pull/2501) - Page to edit or add privacy notices [#3058](https://github.com/ethyca/fides/pull/3058) +- Side navigation bar can now also have children navigation links [#3099](https://github.com/ethyca/fides/pull/3099) ### Changed - The `cursor` pagination strategy now also searches for data outside of the `data_path` when determining the cursor value [#3068](https://github.com/ethyca/fides/pull/3068)