feat(Wizard): added plain styling#12289
feat(Wizard): added plain styling#12289thatblindgeye wants to merge 6 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 two optional props, 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 |
|
Preview: https://pf-react-pr-12289.surge.sh A11y report: https://pf-react-pr-12289-a11y.surge.sh |
| */ | ||
| shouldFocusContent?: boolean; | ||
| /** Adds plain styling to the wizard. */ | ||
| isPlain?: boolean; |
There was a problem hiding this comment.
Curious, does this prop do nothing? Or are style changes applied with pf-m-plain. I ask because it is hard coded below so I am assuming it has no styles associated with it.
If the prop does not do anything, do we need to add it.
There was a problem hiding this comment.
This prop will dictate whether the pf-m-plain class is applied
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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-core/src/components/Wizard/Wizard.tsx`:
- Around line 62-65: The public prop should be renamed to isNoPlain while
keeping isNoPlainOnGlass as a deprecated alias: update the Wizard component
props (the isPlain/isNoPlainOnGlass declarations) to accept isNoPlain?: boolean
and treat isNoPlainOnGlass as optional legacy input (e.g., const
effectiveIsNoPlain = isNoPlain ?? isNoPlainOnGlass) so existing behavior is
preserved; update all internal usages in Wizard (and the other occurrences
referenced around lines ~84-85 and ~190-195) to read effectiveIsNoPlain, and add
a short deprecation note/comment for isNoPlainOnGlass so consumers know to
migrate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05040312-0989-4c99-b9e7-989d6905b328
📒 Files selected for processing (1)
packages/react-core/src/components/Wizard/Wizard.tsx
| /** Adds plain styling to the wizard. */ | ||
| isPlain?: boolean; | ||
| /** @beta Prevents the wizard from automatically applying plain styling when glass theme is enabled. */ | ||
| isNoPlainOnGlass?: boolean; |
There was a problem hiding this comment.
Expose isNoPlain as the public prop (keep isNoPlainOnGlass as alias/deprecated).
The behavior is wired correctly, but the public API currently diverges from the requested isNoPlain prop. This can create avoidable churn for consumers and docs.
Proposed compatibility update
export interface WizardProps extends React.HTMLProps<HTMLDivElement> {
/** Adds plain styling to the wizard. */
isPlain?: boolean;
+ /** Prevents the wizard from automatically applying plain styling when glass theme is enabled. */
+ isNoPlain?: boolean;
/** `@beta` Prevents the wizard from automatically applying plain styling when glass theme is enabled. */
isNoPlainOnGlass?: boolean;
}
export const Wizard = ({
...
isPlain = false,
+ isNoPlain = false,
isNoPlainOnGlass = false,
...wrapperProps
}: WizardProps) => {
+ const noPlain = isNoPlain || isNoPlainOnGlass;
...
<div
className={css(
styles.wizard,
isPlain && styles.modifiers.plain,
- isNoPlainOnGlass && styles.modifiers.noPlain,
+ noPlain && styles.modifiers.noPlain,
className
)}Also applies to: 84-85, 190-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-core/src/components/Wizard/Wizard.tsx` around lines 62 - 65,
The public prop should be renamed to isNoPlain while keeping isNoPlainOnGlass as
a deprecated alias: update the Wizard component props (the
isPlain/isNoPlainOnGlass declarations) to accept isNoPlain?: boolean and treat
isNoPlainOnGlass as optional legacy input (e.g., const effectiveIsNoPlain =
isNoPlain ?? isNoPlainOnGlass) so existing behavior is preserved; update all
internal usages in Wizard (and the other occurrences referenced around lines
~84-85 and ~190-195) to read effectiveIsNoPlain, and add a short deprecation
note/comment for isNoPlainOnGlass so consumers know to migrate.
| */ | ||
| shouldFocusContent?: boolean; | ||
| /** Adds plain styling to the wizard. */ | ||
| isPlain?: boolean; |
There was a problem hiding this comment.
Just curious - why is isNoPlainOnGlass beta but isPlain isn't? I'm not suggesting updating it, I just hadn't thought of these being beta.
There was a problem hiding this comment.
We probably could make both beta; I think at least having the isNo... prop as beta gives us an easier means to remove the prop if needed down the line (without having to deprecate then wait for a breaking release)
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
LGTM. Only comment is that the example doesn't really show any difference. IMO it's fine but we could also use a wizard with a header so it changes a little.
thatblindgeye
force-pushed
the
iss12277_wizardPlain
branch
from
6f8ca89 to
aeaf892
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-core/src/components/Wizard/Wizard.tsx (1)
88-93: Consider warning only once instead of on every render.The current implementation logs the warning on every render when both props are true, which could spam the console if the component re-renders frequently. Consider using a ref to ensure the warning fires only once.
♻️ Proposed fix to warn only once
+import { useEffect, useMemo, useRef, useState } from 'react'; + export const Wizard = ({ ... isPlain = false, isNoPlainOnGlass = false, ...wrapperProps }: WizardProps) => { - if (isPlain && isNoPlainOnGlass) { - // eslint-disable-next-line no-console - console.warn( - `Wizard: 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.` - ); - } + const hasWarnedRef = useRef(false); + if (isPlain && isNoPlainOnGlass && !hasWarnedRef.current) { + // eslint-disable-next-line no-console + console.warn( + `Wizard: 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.` + ); + hasWarnedRef.current = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-core/src/components/Wizard/Wizard.tsx` around lines 88 - 93, The console warning in the Wizard component currently runs on every render when isPlain and isNoPlainOnGlass are both true; change this to only warn once by adding a local ref (e.g., warnedRef = useRef(false)) and moving the warning into a useEffect that checks if isPlain && isNoPlainOnGlass && !warnedRef.current, calls console.warn, then sets warnedRef.current = true; update the code that currently logs inside the render path (the block referencing isPlain and isNoPlainOnGlass) to use this useEffect pattern so the message fires only a single time per mount.
🤖 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/Wizard/Wizard.tsx`:
- Around line 88-93: The console warning in the Wizard component currently runs
on every render when isPlain and isNoPlainOnGlass are both true; change this to
only warn once by adding a local ref (e.g., warnedRef = useRef(false)) and
moving the warning into a useEffect that checks if isPlain && isNoPlainOnGlass
&& !warnedRef.current, calls console.warn, then sets warnedRef.current = true;
update the code that currently logs inside the render path (the block
referencing isPlain and isNoPlainOnGlass) to use this useEffect pattern so the
message fires only a single time per mount.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5e22fd6-92ad-4d09-8986-f2417f228536
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
packages/react-core/package.jsonpackages/react-core/src/components/Wizard/Wizard.tsxpackages/react-core/src/components/Wizard/__tests__/Wizard.test.tsxpackages/react-core/src/components/Wizard/examples/Wizard.mdpackages/react-core/src/components/Wizard/examples/WizardPlain.tsxpackages/react-docs/package.jsonpackages/react-icons/package.jsonpackages/react-styles/package.jsonpackages/react-tokens/package.json
✅ Files skipped from review due to trivial changes (6)
- packages/react-docs/package.json
- packages/react-core/src/components/Wizard/examples/Wizard.md
- packages/react-styles/package.json
- packages/react-core/package.json
- packages/react-tokens/package.json
- packages/react-icons/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react-core/src/components/Wizard/tests/Wizard.test.tsx
- packages/react-core/src/components/Wizard/examples/WizardPlain.tsx
5 participants
What: Closes #12277
Additional issues:
Summary by CodeRabbit
New Features
Tests
Documentation
Chores