chore(miscellaneous): update demo flags from js to ts #12197
Conversation
WalkthroughMultiple markdown demo files had embedded React examples replaced with references to new standalone TypeScript example components; several new TSX demo components were added under Changes
Sequence Diagram(s)(omitted — changes are documentation/demo extraction and new demo components without cross-system control-flow requiring sequence visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
coderabbitai
bot
requested review from
lboehling,
nicolethoen and
thatblindgeye
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-core/src/demos/examples/JumpLinks/JumpLinksWithDrawer.tsx (1)
1-1: MissingReactimport for type annotation.
React.FunctionComponentis used on line 43, butReactis not imported. This will cause a TypeScript/runtime error.🔎 Proposed fix
-import { useContext, useRef, useState } from 'react'; +import React, { useContext, useRef, useState } from 'react';Alternatively, use the shorthand
FCtype:-import { useContext, useRef, useState } from 'react'; +import { useContext, useRef, useState, type FC } from 'react';Then on line 43:
-export const JumpLinksWithDrawer: React.FunctionComponent = () => { +export const JumpLinksWithDrawer: FC = () => {Also applies to: 43-43
🤖 Fix all issues with AI Agents
In @packages/react-core/src/demos/DateTimePicker.md:
- Line 23: Update the markdown code fence attribute to remove the extra space so
it matches the other examples: change the code fence declaration that currently
reads with `file ="examples/DateTimePicker/DateTimeRangePicker.tsx"` to use
`file="examples/DateTimePicker/DateTimeRangePicker.tsx"` (no space before `=`)
in the DateTimePicker demo block so attributes are consistent.
In @packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx:
- Line 47: In the BannerTopBottom demo (BannerTopBottom component), fix the
grammar in the displayed string by replacing the contraction "it's" with the
possessive "its" in the text currently reading "of it's relative line height of
1.5." so the string reads "of its relative line height of 1.5."
- Line 46: In the BannerTopBottom JSX text update the dimension strings to
include a space before the parenthetical values: change "1rem(16px)" to "1rem
(16px)" and "1.5rem(24px)" to "1.5rem (24px)" in the content (the string that
currently reads 'Body text should be Red Hat Text at 1rem(16px). It should have
leading of 1.5rem(24px) because <br />') so the sizing text is properly spaced
and readable.
In
@packages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx:
- Around line 86-88: The end TimePicker used in the InputGroupItem (component
TimePicker with props onToTimeChange and isDisabled={!isValidDate(from)}) lacks
an aria-label; add a descriptive aria-label (e.g., aria-label="End time" or
similar consistent with the start picker) to the TimePicker element to restore
accessibility parity with the start time picker and ensure screen readers can
identify the control.
- Around line 17-20: The toValidator function's parameter needs a type
annotation and its message/operator are inconsistent: add an explicit type for
the date parameter (matching how yyyyMMddFormat and isValidDate expect input,
e.g., Date | string) in the toValidator declaration, and then either change the
comparison from >= to > if the "to" date must be strictly after the "from" date,
or update the error text to "The \"to\" date must be on or after the \"from\"
date" to match the current >= behavior; keep references to isValidDate,
yyyyMMddFormat, toValidator and the from variable when making the change.
- Around line 14-15: The from and to state hooks in DateTimeRangePicker (the
useState calls that produce from, setFrom and to, setTo) lack type annotations;
update them to explicitly type the state as a Date (or nullable Date) — e.g.
Date | undefined or Date | null depending on codebase convention — by adding the
generic to useState so subsequent Date method calls are type-safe and TypeScript
stops inferring undefined.
- Around line 41-58: The handlers onToDateChange and onToTimeChange lack
TypeScript parameter types; add explicit annotations matching the "from"
handlers (e.g., onToDateChange(event: React.SyntheticEvent, inputDate: string,
newToDate: Date | null) and onToTimeChange(event: React.SyntheticEvent, time:
string, hour: number, minute: number)), update any imports (React) if necessary,
and ensure nullability for newToDate is handled consistently before calling
methods like setHours/setMinutes.
- Around line 22-39: Add explicit TypeScript types for the handler parameters:
for onFromDateChange annotate _event as React.SyntheticEvent | undefined,
inputDate as string, and newFromDate as Date | null; for onFromTimeChange
annotate _event as React.SyntheticEvent | undefined, time as string, hour as
number, and minute as number. Keep the existing isValidDate guards (which expect
Date | null), and when setting state use setFrom(newFromDate) instead of
wrapping in new Date() since newFromDate is already a mutated Date object.
In @packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx:
- Line 1: The component uses the React.FunctionComponent type
(React.FunctionComponent) but React is not imported; add the missing default
React import at the top of JumpLinksScrollspy.tsx (so the React symbol is
available for the type annotation used on the component) or alternatively
replace React.FunctionComponent with an imported FC type, ensuring the import
and the component type reference (React or FC) are consistent.
- Around line 27-51: The effect using getResizeObserver does not clean up the
ResizeObserver and also relies on a module-scoped masthead; modify the useEffect
so masthead is a local const (remove the outer let masthead) and capture the
observer returned from getResizeObserver into a variable, then return a cleanup
function that calls observer.disconnect(); keep the existing logic that sets
offset via setOffsetHeight and uses mastheadStyles.masthead and isVertical, but
ensure all DOM queries and the observer are scoped inside the effect and torn
down on unmount or when isVertical changes.
In
@packages/react-core/src/demos/examples/Progress/ProgressWithOnlyIncreasing.tsx:
- Around line 1-2: Add the missing import for accessibilityStyles used by
accessibilityStyles.screenReader: import accessibilityStyles from PatternFly's
accessibility CSS module and place it alongside the existing imports at the top
of the file so the reference in ProgressWithOnlyIncreasing (where
accessibilityStyles.screenReader is used) resolves and prevents a runtime error.
In
@packages/react-core/src/demos/examples/ProgressStepper/ProgressStepperBasic.tsx:
- Around line 16-19: The onStepForward handler allows currentStep to become 5
which is out-of-bounds for the steps array (indices 0-4) and the disabled checks
are hardcoded to 0/5; change the logic in onStepForward (and any other place
referencing the literal 5 such as the disabled checks and any fallback in
setCurrentStep) to compute a maxStep = steps.length - 1 and clamp next between 0
and maxStep via setCurrentStep(Math.min(next, maxStep)); also update the "Step
forward"/"Step back" disabled conditions to use currentStep === maxStep and
currentStep === 0 and replace any hardcoded 5 with maxStep so steps[currentStep]
can never be undefined.
🧹 Nitpick comments (8)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx (1)
54-55: Remove redundant key prop on Card.The
keyprop is already specified on the parentGalleryItem(line 54). Adding it again toCard(line 55) is redundant—React only uses the key on the outermost element in the map iteration.🔎 Proposed fix
<GalleryItem key={index}> - <Card key={index}> + <Card> <CardBody>This is a card</CardBody> </Card> </GalleryItem>packages/react-core/src/demos/examples/JumpLinks/JumpLinksWithDrawer.tsx (2)
23-41: Consider adding TypeScript types forJumpLinksWrapperprops.Since this is a TypeScript migration, adding prop types would improve type safety and maintainability.
🔎 Proposed improvement
+interface JumpLinksWrapperProps { + headings: string[]; +} + -const JumpLinksWrapper = ({ headings }) => { +const JumpLinksWrapper: React.FC<JumpLinksWrapperProps> = ({ headings }) => {
47-47: Consider typing theuseReffor better type safety.- const drawerRef = useRef(undefined); + const drawerRef = useRef<HTMLSpanElement>(null);packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx (1)
67-67: Boolean expression may passfalseinstead ofundefined.
!isVertical && 'stack'evaluates tofalsewhenisVerticalis true. If theorientationprop doesn't expect a boolean, this could cause unexpected behavior.- <Sidebar hasGutter orientation={!isVertical && 'stack'}> + <Sidebar hasGutter orientation={!isVertical ? 'stack' : undefined}>packages/react-core/src/demos/examples/Progress/ProgressBasic.tsx (1)
8-10: Add type annotation and boundary validation.The
nextValueparameter is missing a type annotation. Additionally, while the buttons are disabled at boundaries, the function itself doesn't enforce the 0-100 range constraint if called from elsewhere.🔎 Suggested improvements
- const onProgressUpdate = (nextValue) => { + const onProgressUpdate = (nextValue: number) => { + const clampedValue = Math.max(0, Math.min(100, nextValue)); - setCurrentValue(nextValue); + setCurrentValue(clampedValue); };packages/react-core/src/demos/examples/Progress/ProgressWithOnlyIncreasing.tsx (1)
7-11: Add type annotation for the parameter.The
nextValueparameter is missing a type annotation.🔎 Add type annotation
- const onProgressUpdate = (nextValue) => { + const onProgressUpdate = (nextValue: number) => { if (nextValue > currentValue) { setCurrentValue(nextValue); } };packages/react-core/src/demos/examples/ProgressStepper/ProgressStepperBasic.tsx (2)
16-19: Add type annotations to event handler parameters.The
_eventparameters in bothonStepForwardandonStepBackare missing type annotations.🔎 Add type annotations
- const onStepForward = (_event) => { + const onStepForward = (_event: React.MouseEvent<HTMLButtonElement>) => { const next = currentStep + 1; setCurrentStep(next <= 5 ? next : 4); }; - const onStepBack = (_event) => { + const onStepBack = (_event: React.MouseEvent<HTMLButtonElement>) => { const next = currentStep - 1; setCurrentStep(next > 0 ? next : 0); };Also applies to: 21-24
56-62: Consider usingstep.idfor theidprop for consistency.Line 57 passes
index(a number) to theidprop, while line 58 passesstep.id(a string) totitleId. For consistency and to match the step data structure, consider usingstep.idfor both props.🔎 Suggested change
return ( <ProgressStep - id={index} + id={step.id} titleId={step.id} key={index} variant={variant} isCurrent={index === currentStep} aria-label={ariaLabel} >
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/react-core/src/demos/Banner.mdpackages/react-core/src/demos/DateTimePicker.mdpackages/react-core/src/demos/JumpLinks.mdpackages/react-core/src/demos/ProgressDemo.mdpackages/react-core/src/demos/ProgressStepperDemo.mdpackages/react-core/src/demos/examples/Banner/BannerBasicSticky.tsxpackages/react-core/src/demos/examples/Banner/BannerTopBottom.tsxpackages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsxpackages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsxpackages/react-core/src/demos/examples/JumpLinks/JumpLinksWithDrawer.tsxpackages/react-core/src/demos/examples/Progress/ProgressBasic.tsxpackages/react-core/src/demos/examples/Progress/ProgressWithOnlyIncreasing.tsxpackages/react-core/src/demos/examples/ProgressStepper/ProgressStepperBasic.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
packages/react-core/src/demos/examples/ProgressStepper/ProgressStepperBasic.tsx (4)
packages/react-core/src/layouts/Stack/Stack.tsx (1)
Stack(15-28)packages/react-core/src/layouts/Stack/StackItem.tsx (1)
StackItem(15-28)packages/react-core/src/components/ProgressStepper/ProgressStepper.tsx (1)
ProgressStepper(20-43)packages/react-core/src/components/ProgressStepper/ProgressStep.tsx (1)
ProgressStep(59-116)
packages/react-core/src/demos/examples/Progress/ProgressBasic.tsx (3)
packages/react-core/src/layouts/Stack/Stack.tsx (1)
Stack(15-28)packages/react-core/src/layouts/Stack/StackItem.tsx (1)
StackItem(15-28)packages/react-core/src/components/Progress/Progress.tsx (1)
Progress(164-164)
packages/react-core/src/demos/examples/Banner/BannerBasicSticky.tsx (7)
packages/react-core/src/components/Banner/Banner.tsx (1)
Banner(34-62)packages/react-core/src/layouts/Flex/Flex.tsx (1)
Flex(487-545)packages/react-core/src/components/Page/PageSection.tsx (1)
PageSection(85-137)packages/react-core/src/components/Content/Content.tsx (1)
Content(85-118)packages/react-core/src/layouts/Gallery/Gallery.tsx (1)
Gallery(34-72)packages/react-core/src/components/Card/Card.tsx (1)
Card(155-155)packages/react-core/src/components/Card/CardBody.tsx (1)
CardBody(17-30)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx (6)
packages/react-core/src/layouts/Flex/Flex.tsx (1)
Flex(487-545)packages/react-core/src/components/Banner/Banner.tsx (1)
Banner(34-62)packages/react-core/src/components/Content/Content.tsx (1)
Content(85-118)packages/react-core/src/layouts/Gallery/Gallery.tsx (1)
Gallery(34-72)packages/react-core/src/components/Card/Card.tsx (1)
Card(155-155)packages/react-core/src/components/Card/CardBody.tsx (1)
CardBody(17-30)
packages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx (5)
packages/react-core/src/helpers/datetimeUtils.ts (1)
isValidDate(4-4)packages/react-core/src/components/DatePicker/DatePicker.tsx (2)
yyyyMMddFormat(89-93)DatePicker(357-357)packages/react-core/src/layouts/Flex/Flex.tsx (1)
Flex(487-545)packages/react-core/src/components/InputGroup/InputGroup.tsx (1)
InputGroup(31-33)packages/react-core/src/components/InputGroup/InputGroupItem.tsx (1)
InputGroupItem(19-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (4)
packages/react-core/src/demos/examples/Banner/BannerBasicSticky.tsx (1)
1-55: LGTM! Clean demo implementation.The component correctly demonstrates a sticky banner with responsive layout, proper accessibility attributes, and follows PatternFly patterns.
packages/react-core/src/demos/Banner.md (1)
14-14: LGTM! Documentation correctly references externalized demos.The markdown now properly references the new TypeScript example files, maintaining consistency with the PR's goal of externalizing demo code.
Also applies to: 19-19
packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx (1)
53-123: Component structure and accessibility look good.The render logic properly maps headings to both JumpLinks items and content sections, and includes appropriate accessibility attributes (
aria-label,aria-labelledby,tabIndex).packages/react-core/src/demos/JumpLinks.md (1)
24-25: Documentation updates look correct.The code fences now properly reference the new TypeScript example files with the correct paths and
isFullscreenflag.Also applies to: 33-34
| <Content> | ||
| <h1 id="main-title">Main title</h1> | ||
| <p> | ||
| Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> |
There was a problem hiding this comment.
Add missing space in dimension text.
The dimension should be formatted as "1rem (16px)" with a space between the value and the parenthetical pixel equivalent for better readability.
🔎 Proposed fix
- Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br />
+ Body text should be Red Hat Text at 1rem (16px). It should have leading of 1.5rem (24px) because <br />📝 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.
| Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> | |
| Body text should be Red Hat Text at 1rem (16px). It should have leading of 1.5rem (24px) because <br /> |
🤖 Prompt for AI Agents
In @packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx at line
46, In the BannerTopBottom JSX text update the dimension strings to include a
space before the parenthetical values: change "1rem(16px)" to "1rem (16px)" and
"1.5rem(24px)" to "1.5rem (24px)" in the content (the string that currently
reads 'Body text should be Red Hat Text at 1rem(16px). It should have leading of
1.5rem(24px) because <br />') so the sizing text is properly spaced and
readable.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
it looks like it was maybe already updated when I preview? It's not a huge deal imo, but I would typically also format it with a space
There was a problem hiding this comment.
Mash was one step ahead 😆
| <h1 id="main-title">Main title</h1> | ||
| <p> | ||
| Body text should be Red Hat Text at 1rem(16px). It should have leading of 1.5rem(24px) because <br /> | ||
| of it’s relative line height of 1.5. |
There was a problem hiding this comment.
Fix grammar: use possessive "its" instead of contraction "it's".
The sentence refers to the text's relative line height (possessive), not "it is."
🔎 Proposed fix
- of it's relative line height of 1.5.
+ of its relative line height of 1.5.📝 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.
| of it’s relative line height of 1.5. | |
| of its relative line height of 1.5. |
🤖 Prompt for AI Agents
In @packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx at line
47, In the BannerTopBottom demo (BannerTopBottom component), fix the grammar in
the displayed string by replacing the contraction "it's" with the possessive
"its" in the text currently reading "of it's relative line height of 1.5." so
the string reads "of its relative line height of 1.5."
packages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx
Outdated
Show resolved
Hide resolved
| const toValidator = (date) => | ||
| isValidDate(from) && yyyyMMddFormat(date) >= yyyyMMddFormat(from) | ||
| ? '' | ||
| : 'The "to" date must be after the "from" date'; |
There was a problem hiding this comment.
Add type annotation to validator parameter and clarify validation message.
The date parameter lacks a type annotation. Additionally, the validation uses >= (allowing equal dates), but the error message states "must be after," which suggests strict inequality. Consider either:
- Changing to
>if dates must be strictly after, or - Updating the message to "must be on or after" to match the
>=behavior.
🔎 Proposed fix
- const toValidator = (date) =>
+ const toValidator = (date: Date) =>
isValidDate(from) && yyyyMMddFormat(date) >= yyyyMMddFormat(from)
? ''
- : 'The "to" date must be after the "from" date';
+ : 'The "to" date must be on or after the "from" date';📝 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.
| const toValidator = (date) => | |
| isValidDate(from) && yyyyMMddFormat(date) >= yyyyMMddFormat(from) | |
| ? '' | |
| : 'The "to" date must be after the "from" date'; | |
| const toValidator = (date: Date) => | |
| isValidDate(from) && yyyyMMddFormat(date) >= yyyyMMddFormat(from) | |
| ? '' | |
| : 'The "to" date must be on or after the "from" date'; |
🤖 Prompt for AI Agents
In
@packages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx
around lines 17 - 20, The toValidator function's parameter needs a type
annotation and its message/operator are inconsistent: add an explicit type for
the date parameter (matching how yyyyMMddFormat and isValidDate expect input,
e.g., Date | string) in the toValidator declaration, and then either change the
comparison from >= to > if the "to" date must be strictly after the "from" date,
or update the error text to "The \"to\" date must be on or after the \"from\"
date" to match the current >= behavior; keep references to isValidDate,
yyyyMMddFormat, toValidator and the from variable when making the change.
packages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx
Show resolved
Hide resolved
| import { useState } from 'react'; | ||
| import { Progress, Button, Stack, StackItem } from '@patternfly/react-core'; |
There was a problem hiding this comment.
Missing required import for accessibilityStyles.
Line 26 references accessibilityStyles.screenReader, but the import statement is missing. This will cause a runtime error.
🔎 Add the missing import
import { useState } from 'react';
import { Progress, Button, Stack, StackItem } from '@patternfly/react-core';
+import accessibilityStyles from '@patternfly/react-styles/css/utilities/Accessibility/accessibility';📝 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.
| import { useState } from 'react'; | |
| import { Progress, Button, Stack, StackItem } from '@patternfly/react-core'; | |
| import { useState } from 'react'; | |
| import { Progress, Button, Stack, StackItem } from '@patternfly/react-core'; | |
| import accessibilityStyles from '@patternfly/react-styles/css/utilities/Accessibility/accessibility'; |
🤖 Prompt for AI Agents
In
@packages/react-core/src/demos/examples/Progress/ProgressWithOnlyIncreasing.tsx
around lines 1 - 2, Add the missing import for accessibilityStyles used by
accessibilityStyles.screenReader: import accessibilityStyles from PatternFly's
accessibility CSS module and place it alongside the existing imports at the top
of the file so the reference in ProgressWithOnlyIncreasing (where
accessibilityStyles.screenReader is used) resolves and prevents a runtime error.
| const onStepForward = (_event) => { | ||
| const next = currentStep + 1; | ||
| setCurrentStep(next <= 5 ? next : 4); | ||
| }; |
There was a problem hiding this comment.
Fix inconsistent step bounds logic.
There's an inconsistency in the step boundary handling:
- The
stepsarray has 5 elements (indices 0-4), but the logic allowscurrentStepto reach 5. - Line 18: When
next > 5, it sets the step to 4 instead of 5, creating an inconsistency (if the max is 5, why fall back to 4?). - Line 40:
steps[currentStep]will beundefinedwhencurrentStep === 5, yet the "Step forward" button can advance to this state. - The disabled states check for
currentStep === 0andcurrentStep === 5, suggesting 5 is a valid state, but it's out of the array bounds.
🔎 Recommended fix
If the intent is to have 5 steps (0-4):
const onStepForward = (_event) => {
const next = currentStep + 1;
- setCurrentStep(next <= 5 ? next : 4);
+ setCurrentStep(next <= 4 ? next : 4);
};- <Button onClick={onStepForward} isDisabled={currentStep === 5}>
+ <Button onClick={onStepForward} isDisabled={currentStep === 4}>
Step forward
</Button> <div className={accessibilityStyles.screenReader} aria-live="polite">
- {steps[currentStep] && `On ${steps[currentStep].title}.`}
+ {currentStep < steps.length && `On ${steps[currentStep].title}.`}
{steps[currentStep - 1] && `${steps[currentStep - 1].title} was successful.`}
</div>Also applies to: 29-34, 39-42
🤖 Prompt for AI Agents
In
@packages/react-core/src/demos/examples/ProgressStepper/ProgressStepperBasic.tsx
around lines 16 - 19, The onStepForward handler allows currentStep to become 5
which is out-of-bounds for the steps array (indices 0-4) and the disabled checks
are hardcoded to 0/5; change the logic in onStepForward (and any other place
referencing the literal 5 such as the disabled checks and any fallback in
setCurrentStep) to compute a maxStep = steps.length - 1 and clamp next between 0
and maxStep via setCurrentStep(Math.min(next, maxStep)); also update the "Step
forward"/"Step back" disabled conditions to use currentStep === maxStep and
currentStep === 0 and replace any hardcoded 5 with maxStep so steps[currentStep]
can never be undefined.
There was a problem hiding this comment.
We can tweak any of this logic in a separate issue if we want. It's working as intended: the "Step forward" button remains active while on the last step because clicking it again will update the last step from "in progress" to "complete".
There was a problem hiding this comment.
@thatblindgeye, thank you for the clarification! I understand now — the extra state (currentStep === 5) represents the completion of all steps, where the final step transitions from "in progress" to "complete". That makes perfect sense for the intended UX flow. I apologize for the confusion in my initial review.
✏️ Learnings added
Learnt from: thatblindgeye
Repo: patternfly/patternfly-react PR: 12197
File: packages/react-core/src/demos/examples/ProgressStepper/ProgressStepperBasic.tsx:16-19
Timestamp: 2026-01-08T13:55:13.788Z
Learning: In PatternFly React ProgressStepper components, the currentStep state can validly exceed the last step index (e.g., currentStep === 5 for a 5-step array with indices 0-4). This extra state represents "all steps complete" and allows the last step to transition from "in progress" to "complete" when the user clicks "Step forward" one final time on the last step.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Preview: https://pf-react-pr-12197.surge.sh A11y report: https://pf-react-pr-12197-a11y.surge.sh |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx:
- Line 27: The code assumes DOM nodes exist when calling
document.getElementsByClassName(...)[0] for masthead and sticky; guard against
missing elements by checking the collection length or the returned element
before accessing .offsetHeight (e.g., verify masthead and sticky are truthy
before using masthead.offsetHeight/sticky.offsetHeight in the logic inside
JumpLinksScrollspy), and handle the missing-case safely (skip calculations or
use a default height) so no property access occurs on undefined.
- Line 1: The component uses the React.FunctionComponent type but React is not
imported; add the default React import so the type annotation resolves (i.e.,
import React from 'react') at the top of the file where useEffect/useState are
imported; ensure the import coexists with the existing named imports and update
any ESLint/TS settings if necessary so React.FunctionComponent in
JumpLinksScrollspy is recognized.
🧹 Nitpick comments (1)
packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx (1)
80-85: Remove redundant emptyJumpLinksList.Each
JumpLinksItemcontains an emptyJumpLinksListthat serves no purpose. Consider removing it for cleaner code.🔎 Proposed refactor
{headings.map((i) => ( - <JumpLinksItem key={i} href={`#heading-${i}`}> - {`Heading ${i}`} - <JumpLinksList></JumpLinksList> - </JumpLinksItem> + <JumpLinksItem key={i} href={`#heading-${i}`}> + {`Heading ${i}`} + </JumpLinksItem> ))}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-core/src/demos/DateTimePicker.mdpackages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx (8)
packages/react-core/src/helpers/resizeObserver.tsx (1)
getResizeObserver(70-108)packages/react-core/src/components/Title/Title.tsx (1)
Title(31-50)packages/react-core/src/components/Sidebar/Sidebar.tsx (1)
Sidebar(23-46)packages/react-core/src/components/Sidebar/SidebarPanel.tsx (1)
SidebarPanel(36-60)packages/react-core/src/components/JumpLinks/JumpLinks.tsx (1)
JumpLinks(83-304)packages/react-core/src/components/JumpLinks/JumpLinksItem.tsx (1)
JumpLinksItem(22-50)packages/react-core/src/components/JumpLinks/JumpLinksList.tsx (1)
JumpLinksList(11-19)packages/react-core/src/components/Content/Content.tsx (1)
Content(85-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (1)
packages/react-core/src/demos/DateTimePicker.md (1)
17-17: Spacing consistency issue from past review is now fixed. ✓The external file references are now consistently formatted with no space before the equals sign (
file="..."instead offile ="..."). This matches the desired format and aligns across both code fences.The refactoring to external TypeScript example files is clean and well-executed, successfully moving demo code out of the markdown documentation.
Also applies to: 23-23
|
|
||
| // Update offset based on the masthead and jump links nav heights. | ||
| useEffect(() => { | ||
| const masthead = document.getElementsByClassName(mastheadStyles.masthead)[0]; |
There was a problem hiding this comment.
Guard against missing DOM elements to prevent runtime errors.
The code assumes masthead and sticky elements exist, but getElementsByClassName returns an empty collection if no elements match. Accessing [0] on an empty collection returns undefined, and subsequent property access will throw:
- Line 27: If no masthead element exists,
mastheadisundefined, causing errors on lines 30, 34, 41, and 45 when accessingmasthead.offsetHeight. - Lines 33 and 44: Accessing
.offsetHeightonundefinedthrows immediately, before the truthy check on the next line can prevent further issues.
🔎 Proposed fix
useEffect(() => {
const masthead = document.getElementsByClassName(mastheadStyles.masthead)[0];
+ if (!masthead) return;
if (isVertical) {
setOffsetHeight(masthead.offsetHeight + offsetForPadding);
} else {
// Append jump links nav height to the masthead height when value exists.
- const jumpLinksHeaderHeight = document.getElementsByClassName('pf-m-sticky')[0].offsetHeight;
- jumpLinksHeaderHeight && setOffsetHeight(masthead.offsetHeight + jumpLinksHeaderHeight + offsetForPadding);
+ const jumpLinksHeader = document.getElementsByClassName('pf-m-sticky')[0];
+ if (jumpLinksHeader) {
+ setOffsetHeight(masthead.offsetHeight + jumpLinksHeader.offsetHeight + offsetForPadding);
+ }
}
const observer = getResizeObserver(
document.getElementsByClassName(mastheadStyles.masthead)[0],
() => {
if (isVertical) {
setOffsetHeight(masthead.offsetHeight + offsetForPadding);
} else {
// Append jump links nav height to the masthead height when value exists.
- const jumpLinksHeaderHeight = document.getElementsByClassName('pf-m-sticky')[0].offsetHeight;
- jumpLinksHeaderHeight && setOffsetHeight(masthead.offsetHeight + jumpLinksHeaderHeight + offsetForPadding);
+ const jumpLinksHeader = document.getElementsByClassName('pf-m-sticky')[0];
+ if (jumpLinksHeader) {
+ setOffsetHeight(masthead.offsetHeight + jumpLinksHeader.offsetHeight + offsetForPadding);
+ }
}
},
true
);
return () => observer();
}, [isVertical]);Also applies to: 33-34, 44-45
🤖 Prompt for AI Agents
In @packages/react-core/src/demos/examples/JumpLinks/JumpLinksScrollspy.tsx at
line 27, The code assumes DOM nodes exist when calling
document.getElementsByClassName(...)[0] for masthead and sticky; guard against
missing elements by checking the collection length or the returned element
before accessing .offsetHeight (e.g., verify masthead and sticky are truthy
before using masthead.offsetHeight/sticky.offsetHeight in the logic inside
JumpLinksScrollspy), and handle the missing-case safely (skip calculations or
use a default height) so no property access occurs on undefined.
There was a problem hiding this comment.
For example purposes this should be fine as-is, but wouldn't hurt to make a small update to take into account a scenario where masthead doesn't exist.
There was a problem hiding this comment.
@thatblindgeye Thank you for the clarification! You're absolutely right that for demo purposes the current implementation is fine, especially since the DashboardWrapper should ensure the masthead exists.
If you do decide to add the guard for extra safety, the suggested check at line 27 (if (!masthead) return;) would handle it cleanly. Either way works for this example context.
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Overall this lgtm. There's some open comments from CodeRabbit that we should update (mainly typescript stuff, really anything major/critical it called out and hasn't been resolved/commented on by me). Also pinged Erin about the minor spacing thing in example verbiage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx (1)
17-18: Consider simplifying the component structure.Two optional refactors to improve maintainability:
- The
Fragmentwrapper is unnecessary since it only contains a singleFlexelement.- The top and bottom banners share nearly identical JSX. Consider extracting a reusable banner component or helper function to reduce duplication.
♻️ Proposed refactors
1. Remove unnecessary Fragment:
-export const BannerTopBottom: React.FunctionComponent = () => ( - <Fragment> - <Flex +export const BannerTopBottom: React.FunctionComponent = () => ( + <Flex direction={{ default: 'column' }} flexWrap={{ default: 'nowrap' }} spaceItems={{ default: 'spaceItemsNone' }} style={{ height: '100%' }} > {/* ... rest of content ... */} - </Flex> - </Fragment> -); + </Flex> +);2. Extract banner rendering to reduce duplication:
const StickyBanner: React.FunctionComponent<{ message: string }> = ({ message }) => ( <Banner isSticky> <Flex justifyContent={{ default: 'justifyContentCenter', lg: 'justifyContentSpaceBetween' }} flexWrap={{ default: 'nowrap' }} > <div className={css(display.displayNone, display.displayBlockOnLg)}>Localhost</div> <div className={css(display.displayNone, display.displayBlockOnLg)}> {message} </div> <div className={css(display.displayNoneOnLg)}>Drop some text on mobile, truncate if needed.</div> <div className={css(display.displayNone, display.displayBlockOnLg)}>Ned Username</div> </Flex> </Banner> ); // Then use: <StickyBanner message="This message is sticky to the top of the page." />Also applies to: 26-38, 65-78, 80-81
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsxpackages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/demos/examples/DateTimePicker/DateTimeRangePicker.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx (6)
packages/react-core/src/layouts/Flex/Flex.tsx (1)
Flex(487-545)packages/react-core/src/components/Banner/Banner.tsx (1)
Banner(34-62)packages/react-core/src/components/Page/PageSection.tsx (1)
PageSection(85-137)packages/react-core/src/components/Content/Content.tsx (1)
Content(85-118)packages/react-core/src/layouts/Gallery/Gallery.tsx (1)
Gallery(34-72)packages/react-core/src/components/Card/Card.tsx (1)
Card(155-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (1)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx (1)
17-81: Well-structured demo component!The implementation correctly demonstrates sticky banners with responsive content and follows PatternFly component patterns. The accessibility labels are appropriately applied, and the layout structure is sound.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx:
- Around line 53-59: The mapped elements render duplicate keys: remove the
redundant key prop from the inner Card inside the Array.from(...).map so only
the outer GalleryItem retains the key; locate the map callback that returns
<GalleryItem key={index}> and <Card key={index}> and delete the key on Card
(keep GalleryItem's key).
🧹 Nitpick comments (2)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx (2)
18-18: Consider removing unnecessary Fragment wrapper.The Fragment wrapper is not needed since there's only one root element (the Flex component). You can return the Flex directly.
♻️ Simplify by removing Fragment
- <Fragment> - <Flex + <Flex direction={{ default: 'column' }} flexWrap={{ default: 'nowrap' }} spaceItems={{ default: 'spaceItemsNone' }} style={{ height: '100%' }} > {/* ... */} - </Flex> - </Fragment> + </Flex>If you remove the Fragment, you can also remove the import on line 1.
Also applies to: 80-80
25-39: Optional: Consider extracting banner rendering logic.The top and bottom banners share nearly identical structure, differing only in the message text (lines 33 vs 72). For a demo file, this duplication is acceptable, but extracting a helper function could reduce repetition if this pattern is reused elsewhere.
♻️ Example extraction (if desired)
const StickyBanner: React.FC<{ message: string }> = ({ message }) => ( <Banner isSticky> <Flex justifyContent={{ default: 'justifyContentCenter', lg: 'justifyContentSpaceBetween' }} flexWrap={{ default: 'nowrap' }} > <div className={css(display.displayNone, display.displayBlockOnLg)}>Localhost</div> <div className={css(display.displayNone, display.displayBlockOnLg)}> {message} </div> <div className={css(display.displayNoneOnLg)}>Drop some text on mobile, truncate if needed.</div> <div className={css(display.displayNone, display.displayBlockOnLg)}>Ned Username</div> </Flex> </Banner> );Then use:
<FlexItem> <StickyBanner message="This message is sticky to the top of the page." /> </FlexItem>Also applies to: 64-78
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
| {Array.from({ length: 30 }).map((_value, index) => ( | ||
| <GalleryItem key={index}> | ||
| <Card key={index}> | ||
| <CardBody>This is a card</CardBody> | ||
| </Card> | ||
| </GalleryItem> | ||
| ))} |
There was a problem hiding this comment.
Remove redundant key prop on Card.
The Card component at line 55 has a duplicate key prop. Only the GalleryItem (the direct child in the map) needs a key. The inner Card doesn't require one.
🔧 Remove duplicate key
{Array.from({ length: 30 }).map((_value, index) => (
<GalleryItem key={index}>
- <Card key={index}>
+ <Card>
<CardBody>This is a card</CardBody>
</Card>
</GalleryItem>
))}📝 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.
| {Array.from({ length: 30 }).map((_value, index) => ( | |
| <GalleryItem key={index}> | |
| <Card key={index}> | |
| <CardBody>This is a card</CardBody> | |
| </Card> | |
| </GalleryItem> | |
| ))} | |
| {Array.from({ length: 30 }).map((_value, index) => ( | |
| <GalleryItem key={index}> | |
| <Card> | |
| <CardBody>This is a card</CardBody> | |
| </Card> | |
| </GalleryItem> | |
| ))} |
🤖 Prompt for AI Agents
In @packages/react-core/src/demos/examples/Banner/BannerTopBottom.tsx around
lines 53 - 59, The mapped elements render duplicate keys: remove the redundant
key prop from the inner Card inside the Array.from(...).map so only the outer
GalleryItem retains the key; locate the map callback that returns <GalleryItem
key={index}> and <Card key={index}> and delete the key on Card (keep
GalleryItem's key).
5 participants
Towards #11952
The following demos have been updated:
Summary by CodeRabbit
Documentation
New Features
✏️ Tip: You can customize this high-level summary in your review settings.