feat(Data list): Add isNoPlainOnGlass prop to add pf-m-no-plain modfier to the data list#12292
Open
tlabaj wants to merge 2 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 2 commits intopatternfly:mainfrom
tlabaj wants to merge 2 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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
…ier to the data list
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
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 #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
Bug Fixes
Documentation
Tests