feat(Data list): Add isNoPlainOnGlass prop to add pf-m-no-plain modfier to the data list#12292
Open
tlabaj wants to merge 4 commits intopatternfly:mainfrom
Open
feat(Data list): Add isNoPlainOnGlass prop to add pf-m-no-plain modfier to the data list#12292tlabaj wants to merge 4 commits intopatternfly:mainfrom
tlabaj wants to merge 4 commits intopatternfly:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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-12292.surge.sh A11y report: https://pf-react-pr-12292-a11y.surge.sh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/react-core/src/components/DataList/__tests__/DataList.test.tsx (1)
81-84: Add a complementary negative-path assertion forisNoPlainOnGlass.Consider adding a test that verifies
styles.modifiers.noPlainis not present when the prop is omitted, to lock in opt-in behavior.🧪 Suggested test addition
+test(`Does not render with ${styles.modifiers.noPlain} when isNoPlainOnGlass is false`, () => { + render(<DataList aria-label="list" />); + expect(screen.getByLabelText('list')).not.toHaveClass(styles.modifiers.noPlain); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/DataList/__tests__/DataList.test.tsx` around lines 81 - 84, Add a complementary negative-path test for the DataList component to assert that styles.modifiers.noPlain is not applied when isNoPlainOnGlass is omitted: create a test (e.g., "does not render with noPlain when isNoPlainOnGlass is false/omitted") that renders <DataList aria-label="list" /> (no isNoPlainOnGlass prop) and expects screen.getByLabelText('list') not.toHaveClass(styles.modifiers.noPlain); reference the existing test name and the DataList component and styles.modifiers.noPlain to locate where to add this spec.
🤖 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/DataList/DataListAction.tsx`:
- Around line 10-11: The JSDoc for the id property on DataListActionProps is
incorrect (it calls it a “data list toggle number”); update the comment for the
id field in DataListActionProps (in DataListAction.tsx) to accurately describe
it as the unique identifier for the action (e.g., "Unique identifier for the
DataListAction" or "ID of the action element"), so the JSDoc matches the
interface purpose.
In `@packages/react-core/src/components/DataList/DataListCell.tsx`:
- Around line 12-15: Update the JSDoc for the DataListCell props: replace the
"Width (from 1-5) to the data list cell" comment for the width prop with a
clearer description such as "Width scale for the data list cell (1-5)"; and
replace "Enables the body content to fill the height of the card" for isFilled
with something accurate like "If true, allows the cell content to grow to fill
available height." Edit the JSDoc comments adjacent to the width and isFilled
prop declarations in DataListCell so the wording is concise and precise.
In `@packages/react-core/src/components/DataList/DataListItemCells.tsx`:
- Line 5: The JSDoc for the DataListItemCells component has a typo in the prop
description ("one ore more" → "one or more"); update the comment above the prop
(the JSDoc that mentions "Children should be one ore more <DataListCell> nodes")
in DataListItemCells.tsx so it reads "one or more" and keep the rest of the
phrasing unchanged.
---
Nitpick comments:
In `@packages/react-core/src/components/DataList/__tests__/DataList.test.tsx`:
- Around line 81-84: Add a complementary negative-path test for the DataList
component to assert that styles.modifiers.noPlain is not applied when
isNoPlainOnGlass is omitted: create a test (e.g., "does not render with noPlain
when isNoPlainOnGlass is false/omitted") that renders <DataList
aria-label="list" /> (no isNoPlainOnGlass prop) and expects
screen.getByLabelText('list') not.toHaveClass(styles.modifiers.noPlain);
reference the existing test name and the DataList component and
styles.modifiers.noPlain to locate where to add this spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a752ea0d-05c2-4bd6-b2c7-9360ce1ab859
📒 Files selected for processing (6)
packages/react-core/src/components/DataList/DataList.tsxpackages/react-core/src/components/DataList/DataListAction.tsxpackages/react-core/src/components/DataList/DataListCell.tsxpackages/react-core/src/components/DataList/DataListItemCells.tsxpackages/react-core/src/components/DataList/DataListToggle.tsxpackages/react-core/src/components/DataList/__tests__/DataList.test.tsx
Comment on lines
+10
to
11
| /** Identify the data list toggle number */ | ||
| id: string; |
There was a problem hiding this comment.
id JSDoc is inaccurate for DataListActionProps.
The description references a “data list toggle number,” but this interface is for data list actions.
✏️ Suggested edit
- /** Identify the data list toggle number */
+ /** Identify the data list action */
id: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Identify the data list toggle number */ | |
| id: string; | |
| /** Identify the data list action */ | |
| id: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/DataList/DataListAction.tsx` around lines
10 - 11, The JSDoc for the id property on DataListActionProps is incorrect (it
calls it a “data list toggle number”); update the comment for the id field in
DataListActionProps (in DataListAction.tsx) to accurately describe it as the
unique identifier for the action (e.g., "Unique identifier for the
DataListAction" or "ID of the action element"), so the JSDoc matches the
interface purpose.
packages/react-core/src/components/DataList/DataListItemCells.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-core/src/components/DataList/DataList.tsx (1)
74-79: Warning fires on every render.When both props are true, this warning will log on every component re-render. For a heavily re-rendering component, this could flood the console. Consider guarding with a ref or
useEffectto warn only once per mount.That said, since this is a
@betaprop and users should heed the warning and fix their configuration, the current approach is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/DataList/DataList.tsx` around lines 74 - 79, The warning currently logged inside the render when both isPlain and isNoPlainOnGlass are true should be emitted only once per mount; move the console.warn into a side-effect (e.g., useEffect with an empty dependency array) or guard it with a ref flag so it runs only once. Locate the check using isPlain and isNoPlainOnGlass in the DataList component (DataList.tsx) and replace the inline warning with a useEffect that checks those props and calls console.warn a single time, or implement a useRef boolean (e.g., warnedRef) to prevent repeated logs across re-renders.packages/react-core/src/components/DataList/__tests__/DataList.test.tsx (1)
86-96: Consider adding a test to verify class behavior when both props are true.The warning states that
isPlaintakes precedence andisNoPlainOnGlasswill have no effect. However, the current implementation applies both CSS classes when both props are true. Consider adding a test to explicitly verify the expected class application:test('Applies both plain and noPlain modifiers when both isPlain and isNoPlainOnGlass are true', () => { render(<DataList aria-label="list" isPlain isNoPlainOnGlass />); expect(screen.getByLabelText('list')).toHaveClass(styles.modifiers.plain); expect(screen.getByLabelText('list')).toHaveClass(styles.modifiers.noPlain); });This documents the actual behavior and guards against future changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/DataList/__tests__/DataList.test.tsx` around lines 86 - 96, Add a test that asserts actual CSS class application when both props are true: render the DataList component with isPlain and isNoPlainOnGlass and assert the rendered element (queried by aria-label "list") has both styles.modifiers.plain and styles.modifiers.noPlain classes; update or add a test named like 'Applies both plain and noPlain modifiers when both isPlain and isNoPlainOnGlass are true' to live alongside the existing warning test so the suite documents and guards the current behavior of the DataList component.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-core/src/components/DataList/__tests__/DataList.test.tsx`:
- Around line 86-96: Add a test that asserts actual CSS class application when
both props are true: render the DataList component with isPlain and
isNoPlainOnGlass and assert the rendered element (queried by aria-label "list")
has both styles.modifiers.plain and styles.modifiers.noPlain classes; update or
add a test named like 'Applies both plain and noPlain modifiers when both
isPlain and isNoPlainOnGlass are true' to live alongside the existing warning
test so the suite documents and guards the current behavior of the DataList
component.
In `@packages/react-core/src/components/DataList/DataList.tsx`:
- Around line 74-79: The warning currently logged inside the render when both
isPlain and isNoPlainOnGlass are true should be emitted only once per mount;
move the console.warn into a side-effect (e.g., useEffect with an empty
dependency array) or guard it with a ref flag so it runs only once. Locate the
check using isPlain and isNoPlainOnGlass in the DataList component
(DataList.tsx) and replace the inline warning with a useEffect that checks those
props and calls console.warn a single time, or implement a useRef boolean (e.g.,
warnedRef) to prevent repeated logs across re-renders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c149f1c2-3831-4e09-adcb-aad1dfcb3ca2
📒 Files selected for processing (6)
packages/react-core/src/components/DataList/DataList.tsxpackages/react-core/src/components/DataList/DataListAction.tsxpackages/react-core/src/components/DataList/DataListCell.tsxpackages/react-core/src/components/DataList/DataListItemCells.tsxpackages/react-core/src/components/DataList/DataListToggle.tsxpackages/react-core/src/components/DataList/__tests__/DataList.test.tsx
✅ Files skipped from review due to trivial changes (4)
- packages/react-core/src/components/DataList/DataListAction.tsx
- packages/react-core/src/components/DataList/DataListItemCells.tsx
- packages/react-core/src/components/DataList/DataListToggle.tsx
- packages/react-core/src/components/DataList/DataListCell.tsx
Comment on lines
+74
to
+79
| if (isPlain && isNoPlainOnGlass) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| `DataList: When both isPlain and isNoPlainOnGlass are true, isPlain will take precedence and isNoPlainOnGlass will have no effect. It's recommended to pass only one prop according to the current theme.` | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Following up from the conversation last Friday, since Michael is working on a CSS fix for these 2 classes being both applied we're going to remove the console warning from our react PRs
| /** @beta Flag indicating if DataList should have plain styling with a transparent background */ | ||
| /** @beta Flag indicating if data list should have plain styling with a transparent background */ | ||
| isPlain?: boolean; | ||
| /** @beta Flag to prevent the data list 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.
Suggested change
| /** @beta Flag to prevent the data list from automatically applying plain styling when glass theme is enabled. When both this and isPlain are true, isPlain takes precedence. */ | |
| /** @beta Flag to prevent the data list from automatically applying plain styling when glass theme is enabled. */ |
Related to above comment, we can remove this assuming we pull in Michael's CSS fix
…ier to the data list
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-core/src/components/DataList/DataList.tsx (1)
22-27: Fix duplicated wording in prop docs (data list list).These comments read awkwardly and should be normalized to “data list”.
Suggested wording cleanup
- /** Content rendered inside the data list list */ + /** Content rendered inside the data list */ ... - /** Additional classes added to the data list list */ + /** Additional classes added to the data list */ ... - /** Adds accessible text to the data list list */ + /** Adds accessible text to the data list */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/DataList/DataList.tsx` around lines 22 - 27, The JSDoc comments in DataList.tsx use the duplicated phrase "data list list"; update the prop comments for children, className, and 'aria-label' to use "data list" (e.g., "Content rendered inside the data list", "Additional classes added to the data list", "Adds accessible text to the data list") so the wording is normalized and concise; locate these comments in the DataList component props definitions and replace the duplicated phrasing accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-core/src/components/DataList/DataList.tsx`:
- Around line 22-27: The JSDoc comments in DataList.tsx use the duplicated
phrase "data list list"; update the prop comments for children, className, and
'aria-label' to use "data list" (e.g., "Content rendered inside the data list",
"Additional classes added to the data list", "Adds accessible text to the data
list") so the wording is normalized and concise; locate these comments in the
DataList component props definitions and replace the duplicated phrasing
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86f5a52c-46b0-48cf-a0d5-f14c10172790
📒 Files selected for processing (6)
packages/react-core/src/components/DataList/DataList.tsxpackages/react-core/src/components/DataList/DataListAction.tsxpackages/react-core/src/components/DataList/DataListCell.tsxpackages/react-core/src/components/DataList/DataListItemCells.tsxpackages/react-core/src/components/DataList/DataListToggle.tsxpackages/react-core/src/components/DataList/__tests__/DataList.test.tsx
✅ Files skipped from review due to trivial changes (5)
- packages/react-core/src/components/DataList/DataListAction.tsx
- packages/react-core/src/components/DataList/tests/DataList.test.tsx
- packages/react-core/src/components/DataList/DataListToggle.tsx
- packages/react-core/src/components/DataList/DataListItemCells.tsx
- packages/react-core/src/components/DataList/DataListCell.tsx
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-integration/cypress/integration/datalist.spec.ts`:
- Around line 16-28: The test that asserts a non-transparent background for the
element selected by cy.get('[data-testid="data-list-glass-plain-both"]') is
documented as failing due to a CSS bug; to avoid breaking CI, skip this test
until the CSS is fixed by marking the containing test/it block as skipped (use
it.skip or describe.skip) or wrap the failing assertion in a conditional
feature-flag check, and add a brief comment referencing the tracking issue/ID
and the selector/data-testid so future fixes can find and re-enable the
assertion.
🪄 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: 0d6a70b7-fe78-4797-8500-5b61c7468472
📒 Files selected for processing (1)
packages/react-integration/cypress/integration/datalist.spec.ts
Comment on lines
+16
to
+28
| /** | ||
| * This test fails due to a css bug. | ||
| */ | ||
| cy.get('[data-testid="data-list-glass-plain-both"]').then(($el) => { | ||
| const el = $el[0]; | ||
| const win = el.ownerDocument.defaultView; | ||
| if (!win) { | ||
| throw new Error('expected window'); | ||
| } | ||
| const bg = win.getComputedStyle(el).backgroundColor; | ||
| const fullyTransparent = bg === 'transparent' || bg === 'rgba(0, 0, 0, 0)' || bg === 'rgba(0,0,0,0)'; | ||
| expect(fullyTransparent, `expected non-transparent background, got ${bg}`).to.eq(false); | ||
| }); |
There was a problem hiding this comment.
This test is documented to fail—skip it or fix the CSS bug first.
The comment on lines 16-18 explicitly states "This test fails due to a css bug." Merging a test known to fail will break CI and reduce confidence in the test suite. Either:
- Skip this assertion with
.skipor wrap in a conditional until the CSS bug is resolved - Fix the underlying CSS bug before merging this PR
- Remove the comment if the bug has already been fixed
If the CSS bug is tracked separately, consider referencing the issue number in the comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-integration/cypress/integration/datalist.spec.ts` around lines
16 - 28, The test that asserts a non-transparent background for the element
selected by cy.get('[data-testid="data-list-glass-plain-both"]') is documented
as failing due to a CSS bug; to avoid breaking CI, skip this test until the CSS
is fixed by marking the containing test/it block as skipped (use it.skip or
describe.skip) or wrap the failing assertion in a conditional feature-flag
check, and add a brief comment referencing the tracking issue/ID and the
selector/data-testid so future fixes can find and re-enable the assertion.
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
3 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 #12275
isNoPlainOnGlass; when true, applies thepf-m-no-plainon the root list so plain styling is not forced under the glass theme.isNoPlainOnGlassby assertingstyles.modifiers.noPlain.Additional issues:
Normalize JSDoc wording to “data list” (and small casing tweaks) in DataListAction, DataListCell, DataListItemCells, and DataListToggle — no API or behavior changes there.
Summary by CodeRabbit
New Features
Documentation
Tests