feat(Page): updated logic to allow section grouping (#10650)

* feat(Page): updated logic to allow section grouping * Updated and added unit tests * Added snapshot test * Removed PageNavigation component * PR feedback * Rebased, updated yarn lock * Updated snapshots * Renamed test file * Updated logic to always render PageBody for page nav * Updated tests
patternfly · Jul 15, 2024 · b9181ae · b9181ae
1 parent 38f193a
commit b9181ae
Show file tree

Hide file tree

Showing 21 changed files with 1,247 additions and 12,616 deletions.
diff --git a/packages/react-core/src/components/Page/Page.tsx b/packages/react-core/src/components/Page/Page.tsx
@@ -4,11 +4,12 @@ import { css } from '@patternfly/react-styles';
 import globalBreakpointXl from '@patternfly/react-tokens/dist/esm/global_breakpoint_xl';
 import { debounce, canUseDOM } from '../../helpers/util';
 import { Drawer, DrawerContent, DrawerContentBody, DrawerPanelContent } from '../Drawer';
-import { PageBreadcrumbProps } from './PageBreadcrumb';
+import { PageBreadcrumb, PageBreadcrumbProps } from './PageBreadcrumb';
 import { PageGroup, PageGroupProps } from './PageGroup';
 import { getResizeObserver } from '../../helpers/resizeObserver';
-import { formatBreakpointMods, getBreakpoint, getVerticalBreakpoint } from '../../helpers/util';
+import { getBreakpoint, getVerticalBreakpoint } from '../../helpers/util';
 import { PageContextProvider } from './PageContext';
+import { PageBody } from './PageBody';
 
 export enum PageLayouts {
   vertical = 'vertical',
@@ -263,32 +264,21 @@ class Page extends React.Component<PageProps, PageState> {
     };
 
     let nav = null;
-    if (horizontalSubnav && isHorizontalSubnavWidthLimited) {
+    if (horizontalSubnav) {
       nav = (
-        <div className={css(styles.pageMainNav, styles.modifiers.limitWidth)}>
-          <div className={css(styles.pageMainBody)}>{horizontalSubnav}</div>
+        <div className={css(styles.pageMainSubnav, isHorizontalSubnavWidthLimited && styles.modifiers.limitWidth)}>
+          <PageBody>{horizontalSubnav}</PageBody>
         </div>
       );
-    } else if (horizontalSubnav) {
-      nav = <div className={css(styles.pageMainNav)}>{horizontalSubnav}</div>;
     }
 
     const crumb = breadcrumb ? (
-      <section
-        className={css(
-          styles.pageMainBreadcrumb,
-          isBreadcrumbWidthLimited && styles.modifiers.limitWidth,
-          formatBreakpointMods(
-            breadcrumbProps?.stickyOnBreakpoint,
-            styles,
-            'sticky-',
-            getVerticalBreakpoint(height),
-            true
-          )
-        )}
+      <PageBreadcrumb
+        stickyOnBreakpoint={breadcrumbProps?.stickyOnBreakpoint}
+        isWidthLimited={isBreadcrumbWidthLimited}
       >
-        {isBreadcrumbWidthLimited ? <div className={css(styles.pageMainBody)}>{breadcrumb}</div> : breadcrumb}
-      </section>
+        <PageBody>{breadcrumb}</PageBody>
+      </PageBreadcrumb>
     ) : null;
 
     const isGrouped = isHorizontalSubnavGrouped || isBreadcrumbGrouped || additionalGroupedContent;

diff --git a/packages/react-core/src/components/Page/PageBody.tsx b/packages/react-core/src/components/Page/PageBody.tsx
@@ -0,0 +1,18 @@
+import * as React from 'react';
+import styles from '@patternfly/react-styles/css/components/Page/page';
+import { css } from '@patternfly/react-styles';
+
+export interface PageBodyProps extends React.HTMLProps<HTMLDivElement> {
+  /** Content rendered inside the section */
+  children?: React.ReactNode;
+  /** Additional classes added to the section */
+  className?: string;
+}
+
+export const PageBody: React.FunctionComponent<PageBodyProps> = ({ className, children, ...props }: PageBodyProps) => (
+  <div {...props} className={css(styles.pageMainBody, className)}>
+    {children}
+  </div>
+);
+
+PageBody.displayName = 'PageBody';
diff --git a/packages/react-core/src/components/Page/PageBreadcrumb.tsx b/packages/react-core/src/components/Page/PageBreadcrumb.tsx
@@ -3,6 +3,7 @@ import { css } from '@patternfly/react-styles';
 import styles from '@patternfly/react-styles/css/components/Page/page';
 import { formatBreakpointMods, Mods } from '../../helpers/util';
 import { PageContext } from './PageContext';
+import { PageBody } from './PageBody';
 
 export interface PageBreadcrumbProps extends React.HTMLProps<HTMLElement> {
   /** Additional classes to apply to the PageBreadcrumb */
@@ -26,6 +27,10 @@ export interface PageBreadcrumbProps extends React.HTMLProps<HTMLElement> {
   hasShadowBottom?: boolean;
   /** Flag indicating if the PageBreadcrumb has a scrolling overflow */
   hasOverflowScroll?: boolean;
+  /** @beta Flag indicating whether children passed to the component should be wrapped by a PageBody.
+   * Set this to false in order to pass multiple, custom PageBody's as children.
+   */
+  hasBodyWrapper?: boolean;
   /** Adds an accessible name to the breadcrumb section. Required when the hasOverflowScroll prop is set to true. */
   'aria-label'?: string;
 }
@@ -39,6 +44,7 @@ export const PageBreadcrumb = ({
   hasShadowBottom = false,
   hasOverflowScroll = false,
   'aria-label': ariaLabel,
+  hasBodyWrapper = true,
   ...props
 }: PageBreadcrumbProps) => {
   const { height, getVerticalBreakpoint } = React.useContext(PageContext);
@@ -65,8 +71,7 @@ export const PageBreadcrumb = ({
       aria-label={ariaLabel}
       {...props}
     >
-      {isWidthLimited && <div className={css(styles.pageMainBody)}>{children}</div>}
-      {!isWidthLimited && children}
+      {hasBodyWrapper ? <PageBody>{children}</PageBody> : children}
     </section>
   );
 };

diff --git a/packages/react-core/src/components/Page/PageNavigation.tsx b/packages/react-core/src/components/Page/PageNavigation.tsx
diff --git a/packages/react-core/src/components/Page/PageSection.tsx b/packages/react-core/src/components/Page/PageSection.tsx
@@ -3,6 +3,7 @@ import styles from '@patternfly/react-styles/css/components/Page/page';
 import { css } from '@patternfly/react-styles';
 import { formatBreakpointMods } from '../../helpers/util';
 import { PageContext } from './PageContext';
+import { PageBody } from './PageBody';
 
 export enum PageSectionVariants {
   default = 'default',
@@ -11,7 +12,6 @@ export enum PageSectionVariants {
 
 export enum PageSectionTypes {
   default = 'default',
-  nav = 'nav',
   subNav = 'subnav',
   breadcrumb = 'breadcrumb',
   tabs = 'tabs',
@@ -26,7 +26,7 @@ export interface PageSectionProps extends React.HTMLProps<HTMLDivElement> {
   /** Section background color variant. This will only apply when the type prop has the "default" value. */
   variant?: 'default' | 'secondary';
   /** Section type variant */
-  type?: 'default' | 'nav' | 'subnav' | 'breadcrumb' | 'tabs' | 'wizard';
+  type?: 'default' | 'subnav' | 'breadcrumb' | 'tabs' | 'wizard';
   /** Enables the page section to fill the available vertical space */
   isFilled?: boolean;
   /** Limits the width of the section */
@@ -57,6 +57,10 @@ export interface PageSectionProps extends React.HTMLProps<HTMLDivElement> {
   hasShadowBottom?: boolean;
   /** Flag indicating if the PageSection has a scrolling overflow */
   hasOverflowScroll?: boolean;
+  /** @beta Flag indicating whether children passed to the component should be wrapped by a PageBody.
+   * Set this to false in order to pass multiple, custom PageBody's as children.
+   */
+  hasBodyWrapper?: boolean;
   /** Adds an accessible name to the page section. Required when the hasOverflowScroll prop is set to true.
    * This prop should also be passed in if a heading is not being used to describe the content of the page section.
    */
@@ -67,7 +71,6 @@ export interface PageSectionProps extends React.HTMLProps<HTMLDivElement> {
 
 const variantType = {
   [PageSectionTypes.default]: styles.pageMainSection,
-  [PageSectionTypes.nav]: styles.pageMainNav,
   [PageSectionTypes.subNav]: styles.pageMainSubnav,
   [PageSectionTypes.breadcrumb]: styles.pageMainBreadcrumb,
   [PageSectionTypes.tabs]: styles.pageMainTabs,
@@ -94,6 +97,7 @@ export const PageSection: React.FunctionComponent<PageSectionProps> = ({
   hasOverflowScroll = false,
   'aria-label': ariaLabel,
   component = 'section',
+  hasBodyWrapper = true,
   ...props
 }: PageSectionProps) => {
   const { height, getVerticalBreakpoint } = React.useContext(PageContext);
@@ -127,8 +131,7 @@ export const PageSection: React.FunctionComponent<PageSectionProps> = ({
       {...(hasOverflowScroll && { tabIndex: 0 })}
       aria-label={ariaLabel}
     >
-      {isWidthLimited && <div className={css(styles.pageMainBody)}>{children}</div>}
-      {!isWidthLimited && children}
+      {hasBodyWrapper ? <PageBody>{children}</PageBody> : children}
     </Component>
   );
 };

diff --git a/packages/react-core/src/components/Page/__tests__/Page.test.tsx b/packages/react-core/src/components/Page/__tests__/Page.test.tsx
@@ -10,7 +10,6 @@ import { Breadcrumb, BreadcrumbItem } from '../../Breadcrumb';
 import { Nav, NavList, NavItem } from '../../Nav';
 import { SkipToContent } from '../../SkipToContent';
 import { PageBreadcrumb } from '../PageBreadcrumb';
-import { PageNavigation } from '../PageNavigation';
 import { PageGroup } from '../PageGroup';
 import { Masthead } from '../../Masthead';
 
@@ -228,33 +227,6 @@ describe('Page', () => {
     expect(asFragment()).toMatchSnapshot();
   });
 
-  test('Check page to verify nav is created - PageNavigation syntax', () => {
-    const masthead = <Masthead />;
-    const Sidebar = <PageSidebar isSidebarOpen />;
-
-    const { asFragment } = render(
-      <Page {...props} masthead={masthead} sidebar={Sidebar}>
-        <PageNavigation>
-          <Nav aria-label="Nav" variant="horizontal-subnav">
-            <NavList>
-              <NavItem itemId={0} isActive>
-                System Panel
-              </NavItem>
-              <NavItem itemId={1}>Policy</NavItem>
-              <NavItem itemId={2}>Authentication</NavItem>
-              <NavItem itemId={3}>Network Services</NavItem>
-              <NavItem itemId={4}>Server</NavItem>
-            </NavList>
-          </Nav>
-        </PageNavigation>
-        <PageSection variant="default">Section with default background</PageSection>
-      </Page>
-    );
-
-    expect(screen.getByRole('main')).not.toHaveAttribute('id');
-    expect(asFragment()).toMatchSnapshot();
-  });
-
   test('Check page to verify grouped nav and breadcrumb - new components syntax', () => {
     const masthead = <Masthead />;
     const Sidebar = <PageSidebar isSidebarOpen />;
@@ -272,19 +244,17 @@ describe('Page', () => {
               </BreadcrumbItem>
             </Breadcrumb>
           </PageBreadcrumb>
-          <PageNavigation>
-            <Nav aria-label="Nav" variant="horizontal-subnav">
-              <NavList>
-                <NavItem itemId={0} isActive>
-                  System Panel
-                </NavItem>
-                <NavItem itemId={1}>Policy</NavItem>
-                <NavItem itemId={2}>Authentication</NavItem>
-                <NavItem itemId={3}>Network Services</NavItem>
-                <NavItem itemId={4}>Server</NavItem>
-              </NavList>
-            </Nav>
-          </PageNavigation>
+          <Nav aria-label="Nav" variant="horizontal-subnav">
+            <NavList>
+              <NavItem itemId={0} isActive>
+                System Panel
+              </NavItem>
+              <NavItem itemId={1}>Policy</NavItem>
+              <NavItem itemId={2}>Authentication</NavItem>
+              <NavItem itemId={3}>Network Services</NavItem>
+              <NavItem itemId={4}>Server</NavItem>
+            </NavList>
+          </Nav>
         </PageGroup>
         <PageSection variant="default">Section with default background</PageSection>
       </Page>

diff --git a/packages/react-core/src/components/Page/__tests__/PageBody.test.tsx b/packages/react-core/src/components/Page/__tests__/PageBody.test.tsx
@@ -0,0 +1,34 @@
+import * as React from 'react';
+import { render, screen } from '@testing-library/react';
+import { PageBody } from '../PageBody';
+import styles from '@patternfly/react-styles/css/components/Page/page';
+
+test('Renders without children', () => {
+  render(<PageBody data-testid="page-main-body" />);
+
+  expect(screen.getByTestId('page-main-body')).toBeVisible();
+});
+test('Renders children', () => {
+  render(<PageBody>Test</PageBody>);
+
+  expect(screen.getByText('Test')).toBeVisible();
+});
+test(`Renders with class ${styles.pageMainBody} by default`, () => {
+  render(<PageBody>Test</PageBody>);
+
+  expect(screen.getByText('Test')).toHaveClass(styles.pageMainBody);
+});
+test(`Renders with custom classes when className is passed`, () => {
+  render(<PageBody className="custom-class">Test</PageBody>);
+
+  expect(screen.getByText('Test')).toHaveClass('custom-class');
+});
+test(`Renders with spread props`, () => {
+  render(<PageBody id="custom-id">Test</PageBody>);
+
+  expect(screen.getByText('Test')).toHaveAttribute('id', 'custom-id');
+});
+test('Matches snapshot', () => {
+  const { asFragment } = render(<PageBody>Test</PageBody>);
+  expect(asFragment()).toMatchSnapshot();
+});
diff --git a/packages/react-core/src/components/Page/__tests__/PageBreadcrumb.test.tsx b/packages/react-core/src/components/Page/__tests__/PageBreadcrumb.test.tsx
@@ -1,6 +1,7 @@
 import * as React from 'react';
 import { render, screen } from '@testing-library/react';
 import { PageBreadcrumb } from '../PageBreadcrumb';
+import styles from '@patternfly/react-styles/css/components/Page/page';
 
 describe('page breadcrumb', () => {
   test('Verify basic render', () => {
@@ -32,6 +33,7 @@ describe('page breadcrumb', () => {
     expect(asFragment()).toMatchSnapshot();
   });
 
+  // Old snapshot tests end here. The following tests can be kept if Page test suites need a revamp
   test('Renders without an aria-label by default', () => {
     render(<PageBreadcrumb>test</PageBreadcrumb>);
 
@@ -41,7 +43,7 @@ describe('page breadcrumb', () => {
   test('Renders with the passed aria-label applied', () => {
     render(<PageBreadcrumb aria-label="Test label">test</PageBreadcrumb>);
 
-    expect(screen.getByText('test')).toHaveAccessibleName('Test label');
+    expect(screen.getByText('test').parentElement).toHaveAccessibleName('Test label');
   });
 
   test('Does not log a warning in the console by default', () => {
@@ -71,4 +73,15 @@ describe('page breadcrumb', () => {
 
     expect(consoleWarning).toHaveBeenCalled();
   });
+
+  test('Renders with PageBody wrapper by default', () => {
+    render(<PageBreadcrumb>test</PageBreadcrumb>);
+
+    expect(screen.getByText('test')).toHaveClass(styles.pageMainBody);
+  });
+  test('Does not render with PageBody wrapper when hasBodyWrapper is false', () => {
+    render(<PageBreadcrumb hasBodyWrapper={false}>test</PageBreadcrumb>);
+
+    expect(screen.getByText('test')).not.toHaveClass(styles.pageMainBody);
+  });
 });