From b4d6fac902a5fe624d569e995a7710c1c401ec58 Mon Sep 17 00:00:00 2001 From: Esther Schmitz Date: Wed, 8 Jan 2025 10:23:52 +0100 Subject: [PATCH] fix(ui): ensure main and header container default for fullwidth behaviour is the same (#700) * fix(ui): ensure main and header container default for fullwidth behaviour is the same * fix(template): add fullWidthContent prop to fix typecheck * refactor(ui): make fullWidth default logic less verbose * fix(template): use passed value for fullWidthContent if available, ensure undefined otherwise --------- Co-authored-by: Wowa Barsukov --- apps/template/src/App.tsx | 12 ++++++ .../AppShell/AppShell.component.tsx | 42 +++++-------------- .../src/components/AppShell/AppShell.test.tsx | 38 +++++++++++------ .../HeaderContainer.component.tsx | 2 +- 4 files changed, 50 insertions(+), 44 deletions(-) diff --git a/apps/template/src/App.tsx b/apps/template/src/App.tsx index fce3b5f0f..b089f9467 100644 --- a/apps/template/src/App.tsx +++ b/apps/template/src/App.tsx @@ -12,6 +12,7 @@ import { ErrorBoundary } from "react-error-boundary" interface AppProps { theme?: string embedded?: string | boolean + fullWidthContent?: string | boolean endpoint?: string currentHost?: string } @@ -48,6 +49,16 @@ export const App = (props: AppProps) => { ) } + // ensure that the fullWidthContent prop is a boolean or undefined if not passed + // TODO: this is a workaround to avoid passing a string to the AppShell component but it is ugly. + // We should find a better way to normalize the prop to boolean in some central location. The same applies to the embedded prop. + const calculatedFullWidthContentValue = + props.fullWidthContent === "true" || props.fullWidthContent === true + ? true + : props.fullWidthContent === "false" || props.fullWidthContent === false + ? false + : undefined + return ( { embedded={props.embedded === "true" || props.embedded === true} sideNavigation={null} topNavigation={null} + fullWidthContent={calculatedFullWidthContentValue} > diff --git a/packages/ui-components/src/components/AppShell/AppShell.component.tsx b/packages/ui-components/src/components/AppShell/AppShell.component.tsx index 21db12015..af43e3132 100644 --- a/packages/ui-components/src/components/AppShell/AppShell.component.tsx +++ b/packages/ui-components/src/components/AppShell/AppShell.component.tsx @@ -18,49 +18,27 @@ import { HeaderContainer } from "../HeaderContainer/index" export const AppShell: React.FC = ({ children, className = "", - contentHeading = "", embedded = false, pageHeader = , pageFooter = , + fullWidthContent, // Must be undefined by default, as we need to differentiate between explicitly passed false and not passed at all. sideNavigation, topNavigation, ...props }) => { // Determine whether to pass set fullWidth to true in embedded mode or not: + // In embedded mode (i.e. embedded == true), fullWidthContent should default to true, unless explicitly passed as false. // In non-embedded mode, fullWidthContent should default to false, unless explicitly set to true. - // In embedded mode though, fullWidthContent should default to true, unless explicitly passed as false. - - if (contentHeading && contentHeading.length) { - console.warn( - "AppShell: The contentHeading prop is obsolete and will be removed in a future version. In order to render a content heading, use a ContentHeading element as a child in your main content." - ) - } - const { fullWidthContent } = props - const renderHeaderContainer = ( - pageHeader?: AppShellProps["pageHeader"], - topNavigation?: AppShellProps["topNavigation"] - ) => { - if (!pageHeader && !topNavigation) { - return null - } - return ( - - {pageHeader && typeof pageHeader === "string" ? : pageHeader} - {topNavigation} - {/* Wrap everything except page header and footer and navigations in a main container. Add top margin to MainContainerInner as we are not in embedded mode here. */} - - ) - } + const fullWidthOrDefault = embedded ? fullWidthContent !== false : fullWidthContent === true return ( - {contentHeading || ""} {embedded ? ( <> - {topNavigation && {topNavigation}} + {topNavigation && {topNavigation}} @@ -71,10 +49,14 @@ export const AppShell: React.FC = ({ ) : ( <> - {renderHeaderContainer(pageHeader, topNavigation)} + + {pageHeader && typeof pageHeader === "string" ? : pageHeader} + {topNavigation} + + {/* Wrap everything except page header and footer and navigations in a main container. Add top margin to MainContainerInner as we are not in embedded mode here. */} @@ -101,8 +83,6 @@ export interface AppShellProps extends React.HTMLAttributes { topNavigation?: React.ReactNode /** Optional. If specified expects a `` component. If undefined no side navigation is rendered. */ sideNavigation?: React.ReactNode - /** OBSOLETE: The contentHeading prop is obsolete and will be removed in a future version. In order to render a content heading, use a `` element as a child in your main content. */ - contentHeading?: string /** Optional: Defaults to false. Set embedded to true if app is to be rendered embedded in another app/page. * In this case only the content area and children are rendered, a TopNavigation if passed, but no header/footer or remaining layout components */ embedded?: boolean diff --git a/packages/ui-components/src/components/AppShell/AppShell.test.tsx b/packages/ui-components/src/components/AppShell/AppShell.test.tsx index 0ca4a8728..cde8e25f9 100644 --- a/packages/ui-components/src/components/AppShell/AppShell.test.tsx +++ b/packages/ui-components/src/components/AppShell/AppShell.test.tsx @@ -23,15 +23,6 @@ describe("AppShell", () => { expect(screen.getByTestId("app-shell")).toHaveClass("juno-body") }) - test("logs a deprecation warning to the console when user passed obsolete contentHeading prop", () => { - const consoleWarnSpy = vi.spyOn(console, "warn") - render() - expect(consoleWarnSpy).toHaveBeenCalled() - expect(consoleWarnSpy).toHaveBeenCalledWith( - "AppShell: The contentHeading prop is obsolete and will be removed in a future version. In order to render a content heading, use a ContentHeading element as a child in your main content." - ) - }) - test("renders an app shell with page header passed as String", () => { render() expect(screen.getByTestId("app-shell")).toBeInTheDocument() @@ -117,23 +108,26 @@ describe("AppShell", () => { expect(screen.getByRole("button")).toBeInTheDocument() }) - // The following test whether the MainContainerInner element rendered by AppShell does the right thing depending of props passed to AppShell: - test("does not render a fullwidth main container in non-embedded mode by default", () => { + // The following test whether the MainContainerInner element rendered by AppShell does the right thing depending of props passed to AppShell + // They also check that the HeaderContainer within the AppShell behaves the same as the MainContainerInner + test("does not render a fullwidth main and header container in non-embedded mode by default", () => { render( ) expect(document.querySelector(".juno-main-inner")).not.toHaveClass("juno-main-inner-fullwidth") + expect(document.querySelector(".juno-header-container")).not.toHaveClass("juno-header-container-fullwidth") }) - test("renders a fullwidth main container in non-embedded mode if passed explicitly", () => { + test("renders a fullwidth main and header container in non-embedded mode if passed explicitly", () => { render( ) expect(document.querySelector(".juno-main-inner")).toHaveClass("juno-main-inner-fullwidth") + expect(document.querySelector(".juno-header-container")).toHaveClass("juno-header-container-fullwidth") }) test("renders a fullwidth main inner container in embedded mode by default", () => { @@ -145,6 +139,16 @@ describe("AppShell", () => { expect(document.querySelector(".juno-main-inner")).toHaveClass("juno-main-inner-fullwidth") }) + test("renders a fullwidth main and header container in embedded mode with topnav by default", () => { + render( + }> + + + ) + expect(document.querySelector(".juno-main-inner")).toHaveClass("juno-main-inner-fullwidth") + expect(document.querySelector(".juno-header-container")).toHaveClass("juno-header-container-fullwidth") + }) + test("renders a non-fullwidth, size-restricted main inner container in embedded mode if passed explicitly", () => { render( @@ -154,6 +158,16 @@ describe("AppShell", () => { expect(document.querySelector(".juno-main-inner")).not.toHaveClass("juno-main-inner-fullwidth") }) + test("renders a non-fullwidth, size-restricted main inner and header container in embedded mode with topnav if passed explicitly", () => { + render( + }> + + + ) + expect(document.querySelector(".juno-main-inner")).not.toHaveClass("juno-main-inner-fullwidth") + expect(document.querySelector(".juno-header-container")).not.toHaveClass("juno-header-container-fullwidth") + }) + test("renders a custom className", () => { render() expect(screen.getByTestId("app-shell")).toBeInTheDocument() diff --git a/packages/ui-components/src/components/HeaderContainer/HeaderContainer.component.tsx b/packages/ui-components/src/components/HeaderContainer/HeaderContainer.component.tsx index dd6cd4570..24c7d2917 100644 --- a/packages/ui-components/src/components/HeaderContainer/HeaderContainer.component.tsx +++ b/packages/ui-components/src/components/HeaderContainer/HeaderContainer.component.tsx @@ -25,7 +25,7 @@ export const HeaderContainer: React.FC = ({