Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
"tslib": "^2.8.1"
},
"devDependencies": {
"@patternfly/patternfly": "6.5.0-prerelease.62",
"@patternfly/patternfly": "6.5.0-prerelease.65",
"case-anything": "^3.1.2",
"css": "^3.0.0",
"fs-extra": "^11.3.3"
Expand Down
8 changes: 7 additions & 1 deletion packages/react-core/src/components/Page/Page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,13 @@ class Page extends Component<PageProps, PageState> {
)}
>
{skipToContent}
{variant === 'docked' ? <div className={css(styles.pageDock)}>{masthead}</div> : masthead}
{variant === 'docked' ? (
<div className={css(styles.pageDock)}>
<div className={css(styles.pageDockMain)}>{masthead}</div>
</div>
) : (
masthead
)}
{sidebar}
{notificationDrawer && (
<div className={css(styles.pageDrawer)}>
Expand Down
8 changes: 8 additions & 0 deletions packages/react-core/src/components/Page/PageGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ export interface PageGroupProps extends React.HTMLProps<HTMLDivElement> {
hasOverflowScroll?: boolean;
/** Adds an accessible name to the page group when the hasOverflowScroll prop is set to true. */
'aria-label'?: string;
/** Adds plain styling to the page group. */
isPlain?: boolean;
/** @beta Prevents the page group from automatically applying plain styling when glass theme is enabled. */
isNoPlainOnGlass?: boolean;
}

export const PageGroup = ({
Expand All @@ -38,6 +42,8 @@ export const PageGroup = ({
hasShadowBottom = false,
hasOverflowScroll = false,
'aria-label': ariaLabel,
isPlain = false,
isNoPlainOnGlass = false,
...props
}: PageGroupProps) => {
const { height, getVerticalBreakpoint } = useContext(PageContext);
Expand All @@ -60,6 +66,8 @@ export const PageGroup = ({
hasShadowTop && styles.modifiers.shadowTop,
hasShadowBottom && styles.modifiers.shadowBottom,
hasOverflowScroll && styles.modifiers.overflowScroll,
isPlain && styles.modifiers.plain,
isNoPlainOnGlass && styles.modifiers.noPlainOnGlass,
className
)}
{...(hasOverflowScroll && { tabIndex: 0, role: 'region', 'aria-label': ariaLabel })}
Expand Down
8 changes: 8 additions & 0 deletions packages/react-core/src/components/Page/PageSection.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ export interface PageSectionProps extends React.HTMLProps<HTMLDivElement> {
'aria-label'?: string;
/** Sets the base component to render. Defaults to section */
component?: keyof React.JSX.IntrinsicElements;
/** Adds plain styling to the page section. */
isPlain?: boolean;
/** @beta Prevents the page section from automatically applying plain styling when glass theme is enabled. */
isNoPlainOnGlass?: boolean;
}

const variantType = {
Expand Down Expand Up @@ -98,6 +102,8 @@ export const PageSection: React.FunctionComponent<PageSectionProps> = ({
'aria-label': ariaLabel,
component = 'section',
hasBodyWrapper = true,
isPlain = false,
isNoPlainOnGlass = false,
...props
}: PageSectionProps) => {
const { height, getVerticalBreakpoint } = useContext(PageContext);
Expand Down Expand Up @@ -126,6 +132,8 @@ export const PageSection: React.FunctionComponent<PageSectionProps> = ({
hasShadowTop && styles.modifiers.shadowTop,
hasShadowBottom && styles.modifiers.shadowBottom,
hasOverflowScroll && styles.modifiers.overflowScroll,
isPlain && styles.modifiers.plain,
isNoPlainOnGlass && styles.modifiers.noPlainOnGlass,
Comment on lines +135 to +136
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if PageGroup applies the same modifiers consistently and verify test coverage for the precedence behavior.

# Check PageGroup for consistent implementation
rg -n -A5 'isPlain && styles\.modifiers\.plain' packages/react-core/src/components/Page/

# Check test coverage for combined isPlain + isNoPlainOnGlass scenario
rg -n -B2 -A5 'isPlain.*isNoPlainOnGlass|isNoPlainOnGlass.*isPlain' packages/react-core/src/components/Page/__tests__/

Repository: patternfly/patternfly-react

Length of output: 1213


🏁 Script executed:

# Locate the JSDoc for isPlain and isNoPlainOnGlass in PageSection
rg -n -B10 'isPlain' packages/react-core/src/components/Page/PageSection.tsx | head -80

Repository: patternfly/patternfly-react

Length of output: 1895


🏁 Script executed:

# Look for JSDoc or prop definitions that mention the precedence behavior
rg -n 'When both.*isPlain|precedence' packages/react-core/src/components/Page/

Repository: patternfly/patternfly-react

Length of output: 534


🏁 Script executed:

# Check for any test files that verify the combined isPlain + isNoPlainOnGlass behavior
find packages/react-core/src/components/Page/__tests__ -name '*.test.tsx' -o -name '*.test.ts'

Repository: patternfly/patternfly-react

Length of output: 719


🏁 Script executed:

# Check PageSection.test.tsx for tests with both isPlain and isNoPlainOnGlass
cat packages/react-core/src/components/Page/__tests__/PageSection.test.tsx

Repository: patternfly/patternfly-react

Length of output: 7002


🏁 Script executed:

# Check PageGroup.test.tsx for tests with both isPlain and isNoPlainOnGlass
cat packages/react-core/src/components/Page/__tests__/PageGroup.test.tsx

Repository: patternfly/patternfly-react

Length of output: 3715


Add test coverage for combined isPlain and isNoPlainOnGlass scenario.

The JSDoc documents the intended behavior ("When both this and isPlain are true, isPlain takes precedence"), and the implementation applies both modifier classes consistently in PageSection and PageGroup. However, test coverage is missing for when both props are true simultaneously; currently only individual modifiers are tested.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/react-core/src/components/Page/PageSection.tsx` around lines 135 -
136, Add a unit test that mounts PageSection (and mirror for PageGroup) with
both props isPlain={true} and isNoPlainOnGlass={true} and assert that the plain
modifier (styles.modifiers.plain) is applied and the noPlainOnGlass modifier
(styles.modifiers.noPlainOnGlass) is not applied — this verifies the documented
precedence "isPlain takes precedence" when both flags are set; locate tests
referencing PageSection and PageGroup and augment them or add a new test case
that checks class presence/absence for those two modifiers.

className
)}
{...(hasOverflowScroll && { tabIndex: 0 })}
Expand Down
4 changes: 3 additions & 1 deletion packages/react-core/src/components/Page/PageSidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ export const PageSidebar: React.FunctionComponent<PageSidebarProps> = ({
aria-hidden={!sidebarOpen}
{...props}
>
<PageSidebarContext.Provider value={{ isSidebarOpen: sidebarOpen }}>{children}</PageSidebarContext.Provider>
<PageSidebarContext.Provider value={{ isSidebarOpen: sidebarOpen }}>
<div className={css(styles.pageSidebarMain)}>{children}</div>
</PageSidebarContext.Provider>
</div>
);
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ exports[`PageSidebar should match snapshot (auto-generated) 1`] = `
class="pf-v6-c-page__sidebar ''"
id="page-sidebar"
>
<div>
ReactNode
<div
class="pf-v6-c-page__sidebar-main"
>
<div>
ReactNode
</div>
</div>
</div>
</DocumentFragment>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -406,4 +406,11 @@ describe('Page', () => {
render(<Page data-testid="page"></Page>);
expect(screen.getByTestId('page')).not.toHaveClass(styles.modifiers.dock);
});

test(`Renders with ${styles.pageDockMain} wrapper when variant is docked`, () => {
render(<Page variant="docked" masthead={<>Masthead</>} data-testid="page"></Page>);

const pageDockMain = screen.getByText('Masthead').closest(`.${styles.pageDockMain}`);
expect(pageDockMain).toBeInTheDocument();
});
});
186 changes: 98 additions & 88 deletions packages/react-core/src/components/Page/__tests__/PageGroup.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,92 +2,102 @@ import { render, screen } from '@testing-library/react';
import { PageGroup } from '../PageGroup';
import styles from '@patternfly/react-styles/css/components/Page/page';

describe('page group', () => {
test('Verify basic render', () => {
const { asFragment } = render(<PageGroup>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify top sticky', () => {
const { asFragment } = render(<PageGroup stickyOnBreakpoint={{ default: 'top' }}>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify bottom sticky', () => {
const { asFragment } = render(<PageGroup stickyOnBreakpoint={{ default: 'bottom' }}>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify top shadow', () => {
const { asFragment } = render(<PageGroup hasShadowTop>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify bottom shadow', () => {
const { asFragment } = render(<PageGroup hasShadowBottom>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify overflow scroll', () => {
const { asFragment } = render(<PageGroup hasOverflowScroll>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});

test('Renders without an aria-label by default', () => {
render(<PageGroup>test</PageGroup>);

expect(screen.getByText('test')).not.toHaveAccessibleName('Test label');
});

test('Renders with the passed aria-label applied', () => {
render(
<PageGroup aria-label="Test label" hasOverflowScroll>
test
</PageGroup>
);

expect(screen.getByText('test')).toHaveAccessibleName('Test label');
});

test('Does not log a warning in the console by default', () => {
const consoleWarning = jest.spyOn(console, 'warn').mockImplementation();

render(<PageGroup>test</PageGroup>);

expect(consoleWarning).not.toHaveBeenCalled();
});

test('Does not log a warning in the console when an aria-label is included with hasOverflowScroll', () => {
const consoleWarning = jest.spyOn(console, 'warn').mockImplementation();

render(
<PageGroup hasOverflowScroll aria-label="Test label">
test
</PageGroup>
);

expect(consoleWarning).not.toHaveBeenCalled();
});

test('Logs a warning in the console when an aria-label is not included with hasOverflowScroll', () => {
const consoleWarning = jest.spyOn(console, 'warn').mockImplementation();

render(<PageGroup hasOverflowScroll>test</PageGroup>);

expect(consoleWarning).toHaveBeenCalled();
});

test(`Does not render with ${styles.modifiers.fill} or ${styles.modifiers.noFill} if isFilled is not passed`, () => {
render(<PageGroup>test</PageGroup>);

expect(screen.getByText('test')).not.toHaveClass(styles.modifiers.fill);
expect(screen.getByText('test')).not.toHaveClass(styles.modifiers.noFill);
});

test(`Renders with ${styles.modifiers.fill} if isFilled={true} is passed`, () => {
render(<PageGroup isFilled={true}>test</PageGroup>);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.fill);
});

test(`Renders with ${styles.modifiers.noFill} if isFilled={false} is passed`, () => {
render(<PageGroup isFilled={false}>test</PageGroup>);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.noFill);
});
test('Verify basic render', () => {
const { asFragment } = render(<PageGroup>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify top sticky', () => {
const { asFragment } = render(<PageGroup stickyOnBreakpoint={{ default: 'top' }}>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify bottom sticky', () => {
const { asFragment } = render(<PageGroup stickyOnBreakpoint={{ default: 'bottom' }}>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify top shadow', () => {
const { asFragment } = render(<PageGroup hasShadowTop>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify bottom shadow', () => {
const { asFragment } = render(<PageGroup hasShadowBottom>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});
test('Verify overflow scroll', () => {
const { asFragment } = render(<PageGroup hasOverflowScroll>test</PageGroup>);
expect(asFragment()).toMatchSnapshot();
});

test('Renders without an aria-label by default', () => {
render(<PageGroup>test</PageGroup>);

expect(screen.getByText('test')).not.toHaveAccessibleName('Test label');
});

test('Renders with the passed aria-label applied', () => {
render(
<PageGroup aria-label="Test label" hasOverflowScroll>
test
</PageGroup>
);

expect(screen.getByText('test')).toHaveAccessibleName('Test label');
});

test('Does not log a warning in the console by default', () => {
const consoleWarning = jest.spyOn(console, 'warn').mockImplementation();

render(<PageGroup>test</PageGroup>);

expect(consoleWarning).not.toHaveBeenCalled();
});

test('Does not log a warning in the console when an aria-label is included with hasOverflowScroll', () => {
const consoleWarning = jest.spyOn(console, 'warn').mockImplementation();

render(
<PageGroup hasOverflowScroll aria-label="Test label">
test
</PageGroup>
);

expect(consoleWarning).not.toHaveBeenCalled();
});

test('Logs a warning in the console when an aria-label is not included with hasOverflowScroll', () => {
const consoleWarning = jest.spyOn(console, 'warn').mockImplementation();

render(<PageGroup hasOverflowScroll>test</PageGroup>);

expect(consoleWarning).toHaveBeenCalled();
});

test(`Does not render with ${styles.modifiers.fill} or ${styles.modifiers.noFill} if isFilled is not passed`, () => {
render(<PageGroup>test</PageGroup>);

expect(screen.getByText('test')).not.toHaveClass(styles.modifiers.fill);
expect(screen.getByText('test')).not.toHaveClass(styles.modifiers.noFill);
});

test(`Renders with ${styles.modifiers.fill} if isFilled={true} is passed`, () => {
render(<PageGroup isFilled={true}>test</PageGroup>);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.fill);
});

test(`Renders with ${styles.modifiers.noFill} if isFilled={false} is passed`, () => {
render(<PageGroup isFilled={false}>test</PageGroup>);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.noFill);
});

test(`Renders with ${styles.modifiers.plain} class when isPlain is true`, () => {
render(<PageGroup isPlain>test</PageGroup>);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.plain);
});

test(`Renders with ${styles.modifiers.noPlainOnGlass} class when isNoPlainOnGlass is true`, () => {
render(<PageGroup isNoPlainOnGlass>test</PageGroup>);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.noPlainOnGlass);
});
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,23 @@ test(`Renders with ${styles.modifiers.noFill} if isFilled={false} is passed`, ()

expect(screen.getByRole('main')).toHaveClass(styles.modifiers.noFill);
});

test(`Renders with ${styles.modifiers.plain} class when isPlain is true`, () => {
render(
<PageSection hasBodyWrapper={false} isPlain>
test
</PageSection>
);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.plain);
});

test(`Renders with ${styles.modifiers.noPlainOnGlass} class when isNoPlainOnGlass is true`, () => {
render(
<PageSection hasBodyWrapper={false} isNoPlainOnGlass>
test
</PageSection>
);

expect(screen.getByText('test')).toHaveClass(styles.modifiers.noPlainOnGlass);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { render, screen } from '@testing-library/react';
import { PageSidebar } from '../PageSidebar';
import styles from '@patternfly/react-styles/css/components/Page/page';

test(`Renders with ${styles.pageSidebarMain} wrapper`, () => {
render(<PageSidebar data-testid="sidebar">Test</PageSidebar>);

expect(screen.getByText('Test')).toHaveClass(styles.pageSidebarMain);
});
Loading
Loading