feat(Table): update isPlain to apply no-plain when false#12287
Open
kmcfaul wants to merge 4 commits intopatternfly:mainfrom
Open
feat(Table): update isPlain to apply no-plain when false#12287kmcfaul wants to merge 4 commits intopatternfly:mainfrom
kmcfaul wants to merge 4 commits intopatternfly:mainfrom
Conversation
WalkthroughAdded an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 |
kmcfaul
changed the title
kmcfaul
changed the title
Collaborator
|
Preview: https://pf-react-pr-12287.surge.sh A11y report: https://pf-react-pr-12287-a11y.surge.sh |
tlabaj
reviewed
Mar 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-table/src/components/Table/Table.tsx`:
- Around line 53-54: The Table component currently introduces a new public prop
isNoPlainOnGlass instead of making explicit isPlain={false} produce the
pf-m-no-plain modifier; remove or ignore the public isNoPlainOnGlass usage and
change the class-computation logic in the Table component (where you previously
referenced isNoPlainOnGlass and isPlain) to add the "pf-m-no-plain" class when
the theme is glass AND isPlain === false (strict check for explicit false),
leaving isPlain as an optional prop (do not set a default that masks undefined)
so you can detect an explicit false; update all places that previously used
isNoPlainOnGlass (and relevant className concatenations) to use this new
explicit-check logic and remove the redundant prop from the component API.
- Around line 231-233: The code currently adds both plain and noPlain modifiers
which can conflict; update the class logic so the noPlain modifier
(styles.modifiers.noPlain) is only applied when isNoPlainOnGlass is true AND
isPlain is false (e.g., change the predicate to isNoPlainOnGlass && !isPlain)
while leaving isPlain && styles.modifiers.plain and hasNoInset &&
stylesTreeView.modifiers.noInset unchanged; locate this in the Table component
where isPlain, isNoPlainOnGlass, styles.modifiers.plain and
styles.modifiers.noPlain are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75876afa-0a2b-415e-a198-ab561dd936b1
📒 Files selected for processing (2)
packages/react-table/src/components/Table/Table.tsxpackages/react-table/src/components/Table/__tests__/Table.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-table/src/components/Table/tests/Table.test.tsx
tlabaj
reviewed
Mar 24, 2026
| /** @beta Flag indicating if the table should have plain styling with a transparent background */ | ||
| isPlain?: boolean; | ||
| /** @beta Flag indicating if the table should not have plain styling when in the glass theme */ | ||
| isNoPlainOnGlass?: boolean; |
Contributor
There was a problem hiding this comment.
Maybe isPlainOnGlass is more accurate since setting it to true removes the modifier.
Contributor
Author
There was a problem hiding this comment.
Setting it to true is what sets the no-plain modifier.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react-table/src/components/Table/Table.tsx (1)
95-103:⚠️ Potential issue | 🟠 Major
isPlainprecedence is not actually enforced, and explicitisPlain={false}is still not represented in class logic.At Line 130, the warning says
isNoPlainOnGlassis ignored whenisPlainis true, but Line 239 still addsstyles.modifiers.noPlain, so both modifiers can be emitted together. Also, current logic still does not map explicitisPlain={false}topf-m-no-plainas described in the linked objective.Proposed fix
-const TableBase: React.FunctionComponent<TableProps> = ({ +const TableBase: React.FunctionComponent<TableProps> = (tableProps: TableProps) => { + const { children, className, variant, borders = true, isStickyHeader = false, isPlain = false, isNoPlainOnGlass = false, gridBreakPoint = TableGridBreakpoint.gridMd, 'aria-label': ariaLabel, role = 'grid', innerRef, ouiaId, ouiaSafe = true, isTreeTable = false, isNested = false, isStriped = false, isExpandable = false, hasAnimations: hasAnimationsProp, hasNoInset = false, // eslint-disable-next-line `@typescript-eslint/no-unused-vars` nestedHeaderColumnSpans, selectableRowCaptionText, ...props -}: TableProps) => { + } = tableProps; + + const isPlainExplicitlyFalse = + Object.prototype.hasOwnProperty.call(tableProps, 'isPlain') && tableProps.isPlain === false; + const shouldApplyNoPlain = isNoPlainOnGlass || isPlainExplicitlyFalse; - if (isPlain && isNoPlainOnGlass) { + if (isPlain && shouldApplyNoPlain) { // eslint-disable-next-line no-console console.warn( "Table: 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." ); } @@ - isNoPlainOnGlass && styles.modifiers.noPlain, + !isPlain && shouldApplyNoPlain && styles.modifiers.noPlain,Based on learnings: In PatternFly React,
isPlainshould remain defaulted tofalse, and explicit-false handling should not require changing that default.Also applies to: 127-132, 238-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-table/src/components/Table/Table.tsx` around lines 95 - 103, TableBase's class logic must treat isPlain precedence correctly and detect explicit isPlain={false}; stop defaulting isPlain in the parameter list and instead read it from the props object so you can detect whether it was provided (use Object.prototype.hasOwnProperty.call(props, 'isPlain')). In TableBase, if isPlain === true then ignore isNoPlainOnGlass and do NOT add styles.modifiers.noPlain; otherwise add styles.modifiers.noPlain when either (a) isPlain was explicitly set to false or (b) isNoPlainOnGlass is true and isPlain is not true. Update the class-name construction to use this detection and keep the prop default behavior (treat undefined as false for rendering) while enforcing the precedence rules for isPlain and isNoPlainOnGlass.
🧹 Nitpick comments (1)
packages/react-table/src/components/Table/Table.tsx (1)
127-132: Move warning logic to useEffect to avoid side effects in render.The
console.warncurrently runs on every render when bothisPlainandisNoPlainOnGlassare true, violating React's requirement that render functions remain pure. Side effects like logging should run inuseEffectinstead, which executes after render. SinceuseEffectis already imported, wrapping this warning with dependencies[isPlain, isNoPlainOnGlass]will ensure it only triggers when those props change, not on every render.Suggested refactor
- if (isPlain && isNoPlainOnGlass) { - // eslint-disable-next-line no-console - console.warn( - "Table: 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." - ); - } + useEffect(() => { + if (isPlain && isNoPlainOnGlass) { + // eslint-disable-next-line no-console + console.warn( + "Table: 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." + ); + } + }, [isPlain, isNoPlainOnGlass]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-table/src/components/Table/Table.tsx` around lines 127 - 132, The render currently logs a warning when both isPlain and isNoPlainOnGlass are true, causing a side effect inside the Table component render; move that console.warn into a useEffect inside the Table component (use the existing useEffect import) and run it with dependencies [isPlain, isNoPlainOnGlass] so the warning only fires when those props change, preserving render purity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/react-table/src/components/Table/Table.tsx`:
- Around line 95-103: TableBase's class logic must treat isPlain precedence
correctly and detect explicit isPlain={false}; stop defaulting isPlain in the
parameter list and instead read it from the props object so you can detect
whether it was provided (use Object.prototype.hasOwnProperty.call(props,
'isPlain')). In TableBase, if isPlain === true then ignore isNoPlainOnGlass and
do NOT add styles.modifiers.noPlain; otherwise add styles.modifiers.noPlain when
either (a) isPlain was explicitly set to false or (b) isNoPlainOnGlass is true
and isPlain is not true. Update the class-name construction to use this
detection and keep the prop default behavior (treat undefined as false for
rendering) while enforcing the precedence rules for isPlain and
isNoPlainOnGlass.
---
Nitpick comments:
In `@packages/react-table/src/components/Table/Table.tsx`:
- Around line 127-132: The render currently logs a warning when both isPlain and
isNoPlainOnGlass are true, causing a side effect inside the Table component
render; move that console.warn into a useEffect inside the Table component (use
the existing useEffect import) and run it with dependencies [isPlain,
isNoPlainOnGlass] so the warning only fires when those props change, preserving
render purity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfc79189-f0c1-45d1-bc66-4baff6fd5ff6
📒 Files selected for processing (1)
packages/react-table/src/components/Table/Table.tsx
thatblindgeye
approved these changes
Mar 27, 2026
| isStriped && styles.modifiers.striped, | ||
| isExpandable && styles.modifiers.expandable, | ||
| isPlain && styles.modifiers.plain, | ||
| isNoPlainOnGlass && styles.modifiers.noPlain, |
Contributor
There was a problem hiding this comment.
Approved with the caveat that we're just waiting on core bump to go in to update this class 🙈
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 #12272
isNoPlainOnGlassflag to applypf-m-no-plain.isNoPlainOnGlass.Summary by CodeRabbit
Improvements
Tests