feat(Table): update isPlain to apply no-plain when false#12287
Merged
thatblindgeye merged 5 commits intopatternfly:mainfrom Apr 1, 2026
Merged
feat(Table): update isPlain to apply no-plain when false#12287thatblindgeye merged 5 commits intopatternfly:mainfrom
thatblindgeye merged 5 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:
WalkthroughThe Table component's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
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
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 🙈
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react-table/src/components/Table/Table.tsx (1)
53-54:⚠️ Potential issue | 🟠 Major
isPlain={false}behavior is still split behind a new prop instead of the existing API.At Line 53 and Line 102, adding
isNoPlainOnGlassintroduces a second public toggle; at Line 232 the modifier is driven by that new prop rather than explicitisPlain={false}. This diverges from the PR objective and issue requirement.Suggested direction (keep default pattern, detect explicit false)
-export interface TableProps extends React.HTMLProps<HTMLTableElement>, OUIAProps { +export interface TableProps extends React.HTMLProps<HTMLTableElement>, OUIAProps { @@ - /** `@beta` Flag indicating if the table should not have plain styling when in the glass theme */ - isNoPlainOnGlass?: boolean; @@ -const TableBase: React.FunctionComponent<TableProps> = ({ - children, - className, - variant, - borders = true, - isStickyHeader = false, - isPlain = false, - isNoPlainOnGlass = false, +const TableBase: React.FunctionComponent<TableProps> = (tableProps) => { + const { + children, + className, + variant, + borders = true, + isStickyHeader = false, + isPlain = false, @@ - ...props -}: TableProps) => { + ...props + } = tableProps; + + const isPlainExplicitlyFalse = + Object.prototype.hasOwnProperty.call(tableProps, 'isPlain') && tableProps.isPlain === false; @@ - isPlain && styles.modifiers.plain, - isNoPlainOnGlass && styles.modifiers.noPlainOnGlass, + isPlain && styles.modifiers.plain, + isPlainExplicitlyFalse && styles.modifiers.noPlain,Based on learnings: In PatternFly React,
isPlainshould remain defaulted tofalse; omitting it should still behave likeisPlain={false}.Also applies to: 95-103, 231-233
🤖 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 53 - 54, The new prop isNoPlainOnGlass should be removed and the Table component should honor explicit isPlain={false} instead of introducing a second public toggle; update the Table props and logic (references: isNoPlainOnGlass, isPlain, Table component) so the modifier for the glass theme is applied only when isPlain is explicitly false (detect using a strict check like isPlain === false or checking props.hasOwnProperty('isPlain')), revert any usage that drives behavior from isNoPlainOnGlass to use that explicit isPlain check, and remove the isNoPlainOnGlass declaration and any related conditional branches so the existing API (omitting isPlain behaves like false by default) is preserved.
🤖 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 53-54: The new prop isNoPlainOnGlass should be removed and the
Table component should honor explicit isPlain={false} instead of introducing a
second public toggle; update the Table props and logic (references:
isNoPlainOnGlass, isPlain, Table component) so the modifier for the glass theme
is applied only when isPlain is explicitly false (detect using a strict check
like isPlain === false or checking props.hasOwnProperty('isPlain')), revert any
usage that drives behavior from isNoPlainOnGlass to use that explicit isPlain
check, and remove the isNoPlainOnGlass declaration and any related conditional
branches so the existing API (omitting isPlain behaves like false by default) is
preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dec86a92-a5b0-44dc-8bb7-264a71e17cc6
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
packages/react-core/package.jsonpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-table/src/components/Table/Table.tsxpackages/react-table/src/components/Table/__tests__/Table.test.tsxpackages/react-tokens/package.json
✅ Files skipped from review due to trivial changes (6)
- packages/react-tokens/package.json
- packages/react-docs/package.json
- packages/react-table/src/components/Table/tests/Table.test.tsx
- packages/react-styles/package.json
- packages/react-core/package.json
- packages/react-icons/package.json
kmcfaul
force-pushed
the
glass-updates-table
branch
from
8a28744 to
460611d
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/react-table/src/components/Table/Table.tsx (1)
95-103:⚠️ Potential issue | 🟠 Major
isPlain={false}is still not drivingnoPlainOnGlass, and modifiers can conflict.Line 102 defaults
isPlainin a way that masks whetherfalsewas passed explicitly, and Line 232 only checksisNoPlainOnGlass. This still misses the explicit-falserequirement and allowsplain+noPlainOnGlasstogether.Suggested fix (preserve PatternFly default semantics, add explicit-false detection)
-const TableBase: React.FunctionComponent<TableProps> = ({ - 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) => { +const TableBase: React.FunctionComponent<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; + + const isPlainExplicitlyFalse = + Object.prototype.hasOwnProperty.call(tableProps, 'isPlain') && tableProps.isPlain === false; + const shouldApplyNoPlainOnGlass = (isNoPlainOnGlass || isPlainExplicitlyFalse) && !isPlain; ... - isNoPlainOnGlass && styles.modifiers.noPlainOnGlass, + shouldApplyNoPlainOnGlass && styles.modifiers.noPlainOnGlass,Based on learnings: In PatternFly React,
isPlainshould keep its defaultfalseconvention, and omittedisPlainshould remain equivalent tofalse.Also applies to: 231-232
🤖 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, The problem is defaulting isPlain in the TableBase parameter list masks whether it was passed and allows conflicting modifiers; remove the default in the function signature for isPlain so it can be undefined, then compute a local effectiveIsPlain = isPlain ?? false and use that when applying the no-plain-on-glass logic (e.g. replace the current check that only uses isNoPlainOnGlass with use: if (isNoPlainOnGlass && !effectiveIsPlain) { ... } ), ensuring TableBase, isPlain and isNoPlainOnGlass are the symbols updated so plain and noPlainOnGlass cannot conflict.
🤖 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: The problem is defaulting isPlain in the TableBase
parameter list masks whether it was passed and allows conflicting modifiers;
remove the default in the function signature for isPlain so it can be undefined,
then compute a local effectiveIsPlain = isPlain ?? false and use that when
applying the no-plain-on-glass logic (e.g. replace the current check that only
uses isNoPlainOnGlass with use: if (isNoPlainOnGlass && !effectiveIsPlain) { ...
} ), ensuring TableBase, isPlain and isNoPlainOnGlass are the symbols updated so
plain and noPlainOnGlass cannot conflict.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9de11f4a-2b77-4a1a-837d-ae13b5159c30
📒 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
thatblindgeye
approved these changes
Apr 1, 2026
mcoker
approved these changes
Apr 1, 2026
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
5 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
New Features
isNoPlainOnGlassbeta prop that provides an alternative styling mode, enabling enhanced visual customization while working independently from existing styling options.Tests