feat(ClipboardCopy): added textinput callbacks and props #12180
Conversation
WalkthroughClipboardCopy now accepts new props: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (5)
Comment |
![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
🧹 Nitpick comments (1)
packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx (1)
306-307: Document that these handlers only work for non-inline-compact variants.The forwarding is correct, but note that
onFocusandonBluronly work whenvariantis'inline'or'expansion'since theinline-compactvariant doesn't use aTextInputcomponent (it uses<span>or<code>elements instead).Consider updating the JSDoc comments for these props to mention this limitation:
- /** Callback function when text input is focused */ + /** Callback function when text input is focused. Only applies to 'inline' and 'expansion' variants. */ onFocus?: (event: React.FocusEvent<HTMLInputElement>) => void; - /** Callback function when text input is blurred (focus leaves) */ + /** Callback function when text input is blurred (focus leaves). Only applies to 'inline' and 'expansion' variants. */ onBlur?: (event: React.FocusEvent<HTMLInputElement>) => void;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx(3 hunks)
⏰ 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). (1)
- GitHub Check: Build, test & deploy
🔇 Additional comments (1)
packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx (1)
184-185: LGTM!Correctly excludes the event handlers from the div props spread, ensuring they're only passed to the TextInput component.
| /** Callback function when text input is focused */ | ||
| onFocus?: (event?: any) => void; | ||
| /** Callback function when text input is blurred (focus leaves) */ | ||
| onBlur?: (event?: any) => void; |
There was a problem hiding this comment.
Use proper React event types instead of any.
The event parameter should use React.FocusEvent<HTMLInputElement> for type safety and consistency with React patterns, and the event should be required (not optional) as is standard for React event handlers.
Apply this diff to use proper typing:
- /** Callback function when text input is focused */
- onFocus?: (event?: any) => void;
- /** Callback function when text input is blurred (focus leaves) */
- onBlur?: (event?: any) => void;
+ /** Callback function when text input is focused */
+ onFocus?: (event: React.FocusEvent<HTMLInputElement>) => void;
+ /** Callback function when text input is blurred (focus leaves) */
+ onBlur?: (event: React.FocusEvent<HTMLInputElement>) => void;Note: This matches the pattern used for the onChange prop on line 93, which uses a specific React event type with a required event parameter.
📝 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.
| /** Callback function when text input is focused */ | |
| onFocus?: (event?: any) => void; | |
| /** Callback function when text input is blurred (focus leaves) */ | |
| onBlur?: (event?: any) => void; | |
| /** Callback function when text input is focused */ | |
| onFocus?: (event: React.FocusEvent<HTMLInputElement>) => void; | |
| /** Callback function when text input is blurred (focus leaves) */ | |
| onBlur?: (event: React.FocusEvent<HTMLInputElement>) => void; |
🤖 Prompt for AI Agents
In packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx around
lines 94-97, replace the loose any event types with proper React focus event
types: change both onFocus and onBlur signatures to accept a required event
parameter typed as React.FocusEvent<HTMLInputElement> (e.g. onFocus?: (event:
React.FocusEvent<HTMLInputElement>) => void; and onBlur?: (event:
React.FocusEvent<HTMLInputElement>) => void;). Also ensure the React types are
available by importing React or the FocusEvent type from 'react' at the top of
the file if not already imported.
There was a problem hiding this comment.
This is because the TextInput uses onFocus?: (event?: any) => void; and onBlur?: (event?: any) => void; do we need to update this?
We may need to fix the TextInput as well then.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the TextInput component file
fd -t f "TextInput" --extension tsx --extension ts | head -20
Length of output: 1571
🏁 Script executed:
#!/bin/bash
# Look at the TextInput component's prop definitions, specifically for onFocus and onBlur
ast-grep --pattern $'export interface TextInputProps $$$'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for the TextInput props interface definition
rg -nP --type=ts -A 50 "interface TextInputProps" | head -100
Length of output: 5359
@gautamkrishnar, you're absolutely correct! Looking at the TextInput component, it does define onFocus?: (event?: any) => void; and onBlur?: (event?: any) => void; (lines 84-86 in packages/react-core/src/components/TextInput/TextInput.tsx).
Since ClipboardCopy passes these props directly to TextInput (lines 275-276), they need to be type-compatible. If ClipboardCopy used stricter types like React.FocusEvent<HTMLInputElement>, there would be a TypeScript error when passing them through to TextInput.
For this PR, I recommend keeping the current event?: any types to maintain compatibility with TextInput and keep the scope focused on adding the callbacks functionality.
For a follow-up, it would be valuable to improve the TypeScript types across both components:
- Update TextInput's
onFocusandonBlurto useReact.FocusEvent<HTMLInputElement> - Update ClipboardCopy to match
- This ensures type safety throughout the component hierarchy
Would you like me to create a GitHub issue to track improving these event types in both TextInput and ClipboardCopy as a separate enhancement?
🧬 Code Graph Analysis Results
[ClipboardCopy.tsx]
- File: packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx
- Line range: around the onFocus/onBlur props in ClipboardCopyProps
Snippet:
/** Callback function when text input is focused /
onFocus?: (event?: any) => void;
/* Callback function when text input is blurred (focus leaves) */
onBlur?: (event?: any) => void;
- Purpose: Shows that ClipboardCopy currently defines onFocus/onBlur using event?: any, which is directly relevant to the user comment about TextInput onFocus/onBlur typings and potential need to align or fix typing there.
[TextInput.tsx]
- File: packages/react-core/src/components/TextInput/TextInput.tsx
- Line range: 252-254
Snippet:
export const TextInput = forwardRef((props: TextInputProps, ref: React.Ref) => (
<TextInputBase {...props} innerRef={ref as React.MutableRefObject} />
));
- Purpose: Demonstrates how TextInput is exposed via forwardRef, which is relevant to type compatibility and potential adjustments needed in TextInput props (e.g., onFocus/onBlur typings) when used by ClipboardCopy.
|
Preview: https://pf-react-pr-12180.surge.sh A11y report: https://pf-react-pr-12180-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Nit below, not blocking.
Also, would you mind adding some tests for these new props? We'd want to check that both callbacks aren't called when they shouldn't be, and that they are called when expected (so 4 tests total).
There's a couple tests for the onChange callback already in the test suite that you could use as a jumping off point.
packages/react-core/src/components/ClipboardCopy/ClipboardCopy.tsx
Outdated
Show resolved
Hide resolved
gautamkrishnar
force-pushed
the
fix/12178
branch
from
a355381 to
35512a5
Compare
gautamkrishnar
changed the title
gautamkrishnar
force-pushed
the
fix/12178
branch
from
35512a5 to
ad7d8f0
Compare
gautamkrishnar
changed the title
|
Just updtaed the PR to include text input's |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
I didn't even get to request tests for the other new props you added 😿
(thank you for adding those in in addition to the other tests!)
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
4 participants
What: Closes #12178
This PR adds onBlur, onFocus, name and Id for input box configureable from the clipboardCopy component
Additional issues: n/a
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.