fix(ui): ensure main and header container default for fullwidth behav…

…iour 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 <[email protected]>
cloudoperators · Jan 8, 2025 · b4d6fac · b4d6fac
1 parent 212e32b
commit b4d6fac
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 44 deletions.
diff --git 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,13 +49,24 @@ 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 (
     <QueryClientProvider client={queryClient}>
       <AppShell
         pageHeader="Converged Cloud | App Template"
         embedded={props.embedded === "true" || props.embedded === true}
         sideNavigation={null}
         topNavigation={null}
+        fullWidthContent={calculatedFullWidthContentValue}
       >
         <ErrorBoundary fallbackRender={fallbackRender}>
           <AppContent />

diff --git 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<AppShellProps> = ({
   children,
   className = "",
-  contentHeading = "",
   embedded = false,
   pageHeader = <PageHeader />,
   pageFooter = <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 (
-      <HeaderContainer fullWidth={fullWidthContent === true}>
-        {pageHeader && typeof pageHeader === "string" ? <PageHeader heading={pageHeader} /> : 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. */}
-      </HeaderContainer>
-    )
-  }
+  const fullWidthOrDefault = embedded ? fullWidthContent !== false : fullWidthContent === true
 
   return (
     <AppBody className={className} {...props}>
-      {contentHeading || ""}
       {embedded ? (
         <>
-          {topNavigation && <HeaderContainer>{topNavigation}</HeaderContainer>}
+          {topNavigation && <HeaderContainer fullWidth={fullWidthOrDefault}>{topNavigation}</HeaderContainer>}
           <MainContainer>
             <MainContainerInner
-              fullWidth={fullWidthContent === false ? false : true}
+              fullWidth={fullWidthOrDefault}
               hasSideNav={sideNavigation ? true : false}
               className={`${topNavigation ? "jn-mt-[3.875rem]" : ""}`}
             >
@@ -71,10 +49,14 @@ export const AppShell: React.FC<AppShellProps> = ({
         </>
       ) : (
         <>
-          {renderHeaderContainer(pageHeader, topNavigation)}
+          <HeaderContainer fullWidth={fullWidthOrDefault}>
+            {pageHeader && typeof pageHeader === "string" ? <PageHeader heading={pageHeader} /> : pageHeader}
+            {topNavigation}
+          </HeaderContainer>
+          {/* 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. */}
           <MainContainer>
             <MainContainerInner
-              fullWidth={fullWidthContent === true ? true : false}
+              fullWidth={fullWidthOrDefault}
               hasSideNav={sideNavigation ? true : false}
               className="jn-mt-[3.875rem]"
             >
@@ -101,8 +83,6 @@ export interface AppShellProps extends React.HTMLAttributes<HTMLElement> {
   topNavigation?: React.ReactNode
   /** Optional. If specified expects a `<SideNavigation>` 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 `<ContentHeading>` 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
@@ -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(<AppShell contentHeading="My Content Heading" />)
-    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(<AppShell data-testid="app-shell" pageHeader="My Page Header" />)
     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(
       <AppShell>
         <button></button>
       </AppShell>
     )
     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(
       <AppShell fullWidthContent={true}>
         <button></button>
       </AppShell>
     )
     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(
+      <AppShell embedded topNavigation={<TopNavigation></TopNavigation>}>
+        <button></button>
+      </AppShell>
+    )
+    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(
       <AppShell embedded fullWidthContent={false}>
@@ -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(
+      <AppShell embedded fullWidthContent={false} topNavigation={<TopNavigation></TopNavigation>}>
+        <button></button>
+      </AppShell>
+    )
+    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(<AppShell data-testid="app-shell" className="my-custom-classname" />)
     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
@@ -25,7 +25,7 @@ export const HeaderContainer: React.FC<HeaderContainerProps> = ({
     <div
       className={`
     juno-header-container 
-    ${!fullWidth ? "jn-w-full 2xl:jn-container 2xl:jn-mx-auto" : ""}
+    ${!fullWidth ? "jn-w-full 2xl:jn-container 2xl:jn-mx-auto" : "juno-header-container-fullwidth"}
     ${headerContainerStyles} 
     ${className}`}
       {...props}