feat(Page): added styles for glass#12293
Merged
mcoker merged 8 commits intopatternfly:mainfrom Apr 6, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds inner wrapper divs for docked masthead ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Preview: https://pf-react-pr-12293.surge.sh A11y report: https://pf-react-pr-12293-a11y.surge.sh |
thatblindgeye
force-pushed
the
iss12270_pageGlass
branch
2 times, most recently
from
3458253 to
5a7926c
Compare
Contributor
Author
|
My assumption is the integration tests are failing due to this Core bug? patternfly/patternfly#8243 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/react-core/src/components/Page/examples/PagePlainSections.tsx (1)
48-50: Addrel="noopener noreferrer"for external link security.When using
target="_blank"with external links, it's a security best practice to includerel="noopener noreferrer"to prevent the opened page from accessingwindow.opener.🔒 Proposed fix
- <MastheadLogo href="https://patternfly.org" target="_blank"> + <MastheadLogo href="https://patternfly.org" target="_blank" rel="noopener noreferrer"> Logo </MastheadLogo>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Page/examples/PagePlainSections.tsx` around lines 48 - 50, The external MastheadLogo anchor uses target="_blank" without the security attributes; update the JSX for the MastheadLogo component instance (the MastheadLogo element in PagePlainSections) to include rel="noopener noreferrer" alongside target="_blank" so the opened page cannot access window.opener and to mitigate reverse tabnabbing risks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-core/src/components/Page/__tests__/Page.test.tsx`:
- Around line 410-417: Rename the first duplicate/misleading test title so it
reflects that the test asserts the absence of the pageDockMain wrapper when the
Page is not docked; update the test(...) string that currently reads "Renders
with ${styles.pageDockMain} wrapper when variant is docked" to something like
"Does not render ${styles.pageDockMain} wrapper when variant is not docked" to
avoid duplication with the positive docked case and make it clear this test
checks non-docked behavior (refer to the test block that renders <Page
masthead={<>Masthead</>} data-testid="page"> and uses
screen.getByText('Masthead').closest(`.${styles.pageDockMain}`)).
In `@packages/react-core/src/components/Page/__tests__/PageGroup.test.tsx`:
- Around line 99-102: The test title is incorrect: update the test description
for the test that renders <PageGroup isNoPlainOnGlass> so it references the
isNoPlainOnGlass prop (not isPlain). Locate the test with the template literal
test(`Renders with ${styles.modifiers.noPlainOnGlass} class when isPlain is
true`, ...) and change the string to something like "Renders with
${styles.modifiers.noPlainOnGlass} class when isNoPlainOnGlass is true" so the
title matches the prop under test (PageGroup, isNoPlainOnGlass,
styles.modifiers.noPlainOnGlass).
In `@packages/react-core/src/components/Page/__tests__/PageSection.test.tsx`:
- Around line 193-200: Update the test title to accurately reflect the prop
being tested: change the description string in the test for PageSection from
referencing isPlain to reference isNoPlainOnGlass; locate the test that renders
<PageSection hasBodyWrapper={false} isNoPlainOnGlass> and update its title to
something like "Renders with noPlainOnGlass class when isNoPlainOnGlass is true"
so it matches the assertion that expects styles.modifiers.noPlainOnGlass.
In `@packages/react-core/src/components/Page/Page.tsx`:
- Around line 351-354: The PR imports a missing CSS module causing
styles.pageDockMain (used in Page.tsx within the variant === 'docked' branch) to
be undefined; either add the Page CSS artifact to `@patternfly/react-styles`
exporting the required keys (pageDockMain and pageSidebarMain) or remove/guard
usages of styles.pageDockMain and styles.pageSidebarMain in the Page component
(and other Page-related files) until the CSS module exists; update the import
that references packages/react-styles/css/components/Page/page to point to the
correct file or ensure the package publishes the page CSS with the exact keys
expected by the Page component.
---
Nitpick comments:
In `@packages/react-core/src/components/Page/examples/PagePlainSections.tsx`:
- Around line 48-50: The external MastheadLogo anchor uses target="_blank"
without the security attributes; update the JSX for the MastheadLogo component
instance (the MastheadLogo element in PagePlainSections) to include
rel="noopener noreferrer" alongside target="_blank" so the opened page cannot
access window.opener and to mitigate reverse tabnabbing risks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bac938c5-d927-4bdb-af19-bbf905e56989
⛔ Files ignored due to path filters (3)
packages/react-core/src/components/Page/__tests__/Generated/__snapshots__/PageSidebar.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Page/__tests__/__snapshots__/Page.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Page/__tests__/__snapshots__/PageGroup.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (10)
packages/react-core/src/components/Page/Page.tsxpackages/react-core/src/components/Page/PageGroup.tsxpackages/react-core/src/components/Page/PageSection.tsxpackages/react-core/src/components/Page/PageSidebar.tsxpackages/react-core/src/components/Page/__tests__/Page.test.tsxpackages/react-core/src/components/Page/__tests__/PageGroup.test.tsxpackages/react-core/src/components/Page/__tests__/PageSection.test.tsxpackages/react-core/src/components/Page/__tests__/PageSidebar.test.tsxpackages/react-core/src/components/Page/examples/Page.mdpackages/react-core/src/components/Page/examples/PagePlainSections.tsx
Comment on lines
+410
to
+417
| test(`Renders with ${styles.pageDockMain} wrapper when variant is docked`, () => { | ||
| render(<Page masthead={<>Masthead</>} data-testid="page"></Page>); | ||
|
|
||
| const pageDockMain = screen.getByText('Masthead').closest(`.${styles.pageDockMain}`); | ||
| expect(pageDockMain).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test(`Renders with ${styles.pageDockMain} wrapper when variant is docked`, () => { |
There was a problem hiding this comment.
Rename the first dock-wrapper test; current title is misleading and duplicated.
The first new test verifies the absence of pageDockMain when variant is not docked, but its title duplicates the docked-positive case.
Proposed test-title fix
-test(`Renders with ${styles.pageDockMain} wrapper when variant is docked`, () => {
+test(`Does not render ${styles.pageDockMain} wrapper when variant is not docked`, () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Page/__tests__/Page.test.tsx` around lines
410 - 417, Rename the first duplicate/misleading test title so it reflects that
the test asserts the absence of the pageDockMain wrapper when the Page is not
docked; update the test(...) string that currently reads "Renders with
${styles.pageDockMain} wrapper when variant is docked" to something like "Does
not render ${styles.pageDockMain} wrapper when variant is not docked" to avoid
duplication with the positive docked case and make it clear this test checks
non-docked behavior (refer to the test block that renders <Page
masthead={<>Masthead</>} data-testid="page"> and uses
screen.getByText('Masthead').closest(`.${styles.pageDockMain}`)).
packages/react-core/src/components/Page/__tests__/PageGroup.test.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/Page/__tests__/PageSection.test.tsx
Outdated
Show resolved
Hide resolved
thatblindgeye
force-pushed
the
iss12270_pageGlass
branch
from
7d90f05 to
631e4e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-core/src/components/Page/PageSection.tsx`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cacd466-efa9-4e39-bb7b-27c9c806c35e
⛔ Files ignored due to path filters (4)
packages/react-core/src/components/Page/__tests__/Generated/__snapshots__/PageSidebar.test.tsx.snapis excluded by!**/*.snap,!**/generated/**packages/react-core/src/components/Page/__tests__/__snapshots__/Page.test.tsx.snapis excluded by!**/*.snappackages/react-core/src/components/Page/__tests__/__snapshots__/PageGroup.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/react-core/package.jsonpackages/react-core/src/components/Page/Page.tsxpackages/react-core/src/components/Page/PageGroup.tsxpackages/react-core/src/components/Page/PageSection.tsxpackages/react-core/src/components/Page/PageSidebar.tsxpackages/react-core/src/components/Page/__tests__/Page.test.tsxpackages/react-core/src/components/Page/__tests__/PageGroup.test.tsxpackages/react-core/src/components/Page/__tests__/PageSection.test.tsxpackages/react-core/src/components/Page/__tests__/PageSidebar.test.tsxpackages/react-core/src/components/Page/examples/Page.mdpackages/react-core/src/components/Page/examples/PagePlainSections.tsxpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
✅ Files skipped from review due to trivial changes (9)
- packages/react-styles/package.json
- packages/react-tokens/package.json
- packages/react-icons/package.json
- packages/react-docs/package.json
- packages/react-core/src/components/Page/tests/PageSection.test.tsx
- packages/react-core/src/components/Page/tests/PageGroup.test.tsx
- packages/react-core/package.json
- packages/react-core/src/components/Page/tests/PageSidebar.test.tsx
- packages/react-core/src/components/Page/examples/Page.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react-core/src/components/Page/tests/Page.test.tsx
- packages/react-core/src/components/Page/PageSidebar.tsx
- packages/react-core/src/components/Page/PageGroup.tsx
- packages/react-core/src/components/Page/examples/PagePlainSections.tsx
- packages/react-core/src/components/Page/Page.tsx
Comment on lines
+135
to
+136
| isPlain && styles.modifiers.plain, | ||
| isNoPlainOnGlass && styles.modifiers.noPlainOnGlass, |
There was a problem hiding this comment.
🧩 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 -80Repository: 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.tsxRepository: 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.tsxRepository: 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.
kmcfaul
requested changes
Apr 6, 2026
| '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. When both this and isPlain are true, isPlain takes precedence. */ |
Contributor
There was a problem hiding this comment.
The note about both flags should be removed from the description.
| 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. When both this and isPlain are true, isPlain takes precedence. */ |
Collaborator
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What: Closes #12270
Waiting on core bump to go in to pull in correct styles object properties
Additional issues:
Summary by CodeRabbit
New Features
Refactor
Tests