feat(Page): added styles for glass#12293
Open
thatblindgeye wants to merge 6 commits intopatternfly:mainfrom
Open
feat(Page): added styles for glass#12293thatblindgeye wants to merge 6 commits intopatternfly:mainfrom
thatblindgeye wants to merge 6 commits intopatternfly:mainfrom
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 (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds wrapper elements for docked masthead and sidebar content, and introduces 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
from
f154e69 to
e2c2040
Compare
thatblindgeye
force-pushed
the
iss12270_pageGlass
branch
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
Comment on lines
+351
to
+354
| {variant === 'docked' ? ( | ||
| <div className={css(styles.pageDock)}> | ||
| <div className={css(styles.pageDockMain)}>{masthead}</div> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking Page component references..."
rg -n 'styles\.pageDockMain|styles\.pageSidebarMain' packages/react-core/src/components/Page
echo "Locating generated Page style artifacts..."
fd -i 'page.*' | rg -n 'components[/\\]Page[/\\]page(\.|$)|css/components/Page/page(\.|$)' || true
echo "Verifying style keys exist in available artifacts..."
fd -i 'page.*' | xargs -r rg -n 'pageDockMain|pageSidebarMain'Repository: patternfly/patternfly-react
Length of output: 4154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding imports of styles in Page.tsx ==="
head -50 packages/react-core/src/components/Page/Page.tsx | grep -n "import.*styles\|from.*styles"
echo -e "\n=== Checking where styles object comes from ==="
rg -n "^import.*styles" packages/react-core/src/components/Page/Page.tsx | head -5
echo -e "\n=== Locating style definition files ==="
fd -i "page\.scss|page\.css|page\.module" packages/
echo -e "\n=== Checking for style type definitions ==="
fd -type f -name "*.d.ts" -path "*/Page/*" | head -20
echo -e "\n=== Searching for pageDockMain in type definitions ==="
rg -n "pageDockMain" --type ts packages/ 2>/dev/null | head -20Repository: patternfly/patternfly-react
Length of output: 687
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking package.json for `@patternfly/react-styles` version ==="
rg -A2 '"@patternfly/react-styles"' package.json packages/react-core/package.json 2>/dev/null | head -10
echo -e "\n=== Checking if node_modules is available ==="
ls -la node_modules/@patternfly/react-styles/css/components/Page/ 2>/dev/null || echo "node_modules not available"
echo -e "\n=== Searching for TypeScript type definitions for page styles ==="
fd page packages/react-core/src -type f | grep -E "\.(d\.ts|tsx)$" | head -10
echo -e "\n=== Checking test file to see if pageDockMain is defined in a type ==="
rg -B5 "pageDockMain" packages/react-core/src/components/Page/__tests__/Page.test.tsx | head -30
echo -e "\n=== Checking if styles object has a type annotation ==="
rg -A3 "import styles from" packages/react-core/src/components/Page/Page.tsx
echo -e "\n=== Looking for any .d.ts or type stubs related to page ==="
fd "page" packages/react-core -name "*.d.ts"Repository: patternfly/patternfly-react
Length of output: 686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Finding `@patternfly/react-styles` in the monorepo ==="
find . -maxdepth 3 -type d -name "react-styles" 2>/dev/null
echo -e "\n=== Checking directory structure in packages/ ==="
ls -la packages/ | grep -E "^d"
echo -e "\n=== Looking for page.css or page.scss in the entire repo ==="
fd -e css -e scss "page" . 2>/dev/null | grep -v node_modules | head -20
echo -e "\n=== Searching for pageDockMain definition anywhere ==="
rg "pageDockMain" . --type-list 2>/dev/null | head -5
rg "pageDockMain\s*[:=]" . --no-heading 2>/dev/null | head -20Repository: patternfly/patternfly-react
Length of output: 1377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Exploring packages/react-styles structure ==="
ls -la packages/react-styles/src/
echo -e "\n=== Looking for page-related CSS files ==="
find packages/react-styles -name "*page*" -type f 2>/dev/null
echo -e "\n=== Checking CSS components directory ==="
find packages/react-styles -path "*/css/components/Page*" -o -path "*/sass/components/Page*" 2>/dev/null | head -20
echo -e "\n=== Searching for pageDockMain in react-styles package ==="
rg "pageDockMain" packages/react-styles/ 2>/dev/null
echo -e "\n=== Searching for page styles file ==="
find packages/react-styles -name "page.*" -type f 2>/dev/nullRepository: patternfly/patternfly-react
Length of output: 485
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Listing packages/react-styles/src/css ==="
ls -la packages/react-styles/src/css/
echo -e "\n=== Checking for components subdirectory ==="
find packages/react-styles/src/css -type d -name "components" -o -type d -name "Page*"
echo -e "\n=== Listing all files in react-styles/src/css ==="
find packages/react-styles/src/css -type f 2>/dev/null | head -30
echo -e "\n=== Searching entire react-styles for pageDockMain or pageSidebarMain ==="
rg "pageDockMain|pageSidebarMain" packages/react-styles/ 2>/dev/null
echo -e "\n=== Check if CSS files are compiled or in a different format ==="
find packages/react-styles -name "*.css" -o -name "*.scss" 2>/dev/null | head -20Repository: patternfly/patternfly-react
Length of output: 729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Listing all subdirectories in components ==="
ls -la packages/react-styles/src/css/components/
echo -e "\n=== Check for Page directory specifically ==="
ls -la packages/react-styles/src/css/components/Page 2>&1
echo -e "\n=== List all CSS files in react-styles ==="
find packages/react-styles -name "*.css" -type f
echo -e "\n=== Check the index.ts to understand how styles are exported ==="
cat packages/react-styles/src/index.tsRepository: patternfly/patternfly-react
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading the relevant test section ==="
sed -n '410,422p' packages/react-core/src/components/Page/__tests__/Page.test.tsx
echo -e "\n=== Reading more context around the test ==="
sed -n '408,425p' packages/react-core/src/components/Page/__tests__/Page.test.tsxRepository: patternfly/patternfly-react
Length of output: 1409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if page.css/page.scss exists in a different location ==="
find packages/react-styles -name "*.scss" -o -name "*.css" 2>/dev/null
echo -e "\n=== Check the PR or commit messages for context ==="
git log --oneline -20 2>/dev/null | grep -i "page\|dock\|style" || echo "Git log not available"
echo -e "\n=== Verify that import path will fail ==="
rg "@patternfly/react-styles/css/components/Page/page" packages/react-core/
echo -e "\n=== Check if there are any TypeScript type stubs or mock files ==="
find packages -name "*.d.ts" -path "*/Page/*" 2>/dev/null | xargs grep -l "pageDockMain" 2>/dev/null || echo "No type stubs found"
echo -e "\n=== Double check: does the import path actually resolve? ==="
ls -la packages/react-styles/css/components/Page/ 2>&1 || echo "Path confirmed missing"
</parameter>Repository: patternfly/patternfly-react
Length of output: 2671
Block this PR until the Page CSS file is available in @patternfly/react-styles.
The CSS file packages/react-styles/css/components/Page/page does not exist. The import at line 2 of Page.tsx (and 13 other Page-related files) will fail, and styles.pageDockMain referenced at line 353 is undefined. The corresponding CSS component directory is missing from the react-styles package entirely.
This change cannot proceed until the style artifact includes the Page component CSS file with the pageDockMain and pageSidebarMain keys.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Page/Page.tsx` around lines 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.
Contributor
Author
There was a problem hiding this comment.
The file does exist, I have no issues locally.
There was a problem hiding this comment.
@thatblindgeye, you're absolutely right — I apologize for the false positive. My analysis was flawed because the sandbox environment used for script execution doesn't have node_modules available, so I incorrectly concluded the file was missing. @patternfly/react-styles is an external npm package, and @patternfly/react-styles/css/components/Page/page resolves through the installed package — not from within the repository source tree. The CSS file and its exported keys (pageDockMain, pageSidebarMain) exist in your locally installed dependency, as expected.
✏️ Learnings added
Learnt from: thatblindgeye
Repo: patternfly/patternfly-react PR: 12293
File: packages/react-core/src/components/Page/Page.tsx:351-354
Timestamp: 2026-04-01T15:54:35.860Z
Learning: In the patternfly/patternfly-react repository, style imports like `import styles from 'patternfly/react-styles/css/components/Page/page'` resolve to the installed `patternfly/react-styles` npm package in `node_modules`, not to files tracked within the repository source tree. The sandbox environment lacks `node_modules`, so searching for these paths within the repo will always fail. Do not flag missing CSS files from `patternfly/react-styles` as errors based on repo-level file searches.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: arpanroy41
Repo: patternfly/patternfly-react PR: 12237
File: packages/react-core/src/components/Toolbar/examples/ToolbarFlexGrowAndWidths.tsx:6-6
Timestamp: 2026-02-06T19:58:49.801Z
Learning: In the PatternFly React repository, React may be used without an explicit import in TSX files because React is provided globally by the build configuration. You can reference React types (e.g., React.FunctionComponent, React.ReactNode) without importing React. Do not add 'import React from 'react'' in TSX files; rely on the global React availability.
Learnt from: nicolethoen
Repo: patternfly/patternfly-react PR: 12251
File: packages/react-core/src/components/FormSelect/FormSelect.tsx:76-77
Timestamp: 2026-02-26T22:33:56.823Z
Learning: For PatternFly React components using SSRSafeIds, ensure that ouiaComponentType is a stable string (typically the component's displayName) that does not change during the component's lifecycle. Do not include dynamic props such as validation state in ouiaComponentType, as this will make the generated OUIA ID unstable. If you need to expose dynamic state for testing or UI logic, use separate data attributes instead of embedding them in ouiaComponentType.
Learnt from: tlabaj
Repo: patternfly/patternfly-react PR: 12288
File: packages/react-core/src/components/Accordion/__tests__/Accordion.test.tsx:144-150
Timestamp: 2026-03-24T14:47:35.816Z
Learning: In PatternFly React components, treat the `isPlain` prop as a public prop that follows the existing PatternFly convention: its default behavior is `false`. If `isPlain` is omitted by a consumer, it should be treated as equivalent to `isPlain={false}`. During code review, do not recommend changing this default to `undefined` or modifying any `!isPlain`-based conditional logic, since doing so would diverge from established PatternFly API patterns and could introduce a behavioral breaking change.
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
2 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
Documentation