-
Notifications
You must be signed in to change notification settings - Fork 376
fix(demos): fix AttributeValueFiltering menu selection #12207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…rd handling - Fixes Enter/click on attribute reliably opening value list for all attributes - Implement optional chaining to avoid potential undefined errors - Prevents TypeError from undefined menuItemsText - Overall typescript improvements Signed-off-by: Mohamed Fall <[email protected]>
WalkthroughThe PR fixes TypeErrors in the AttributeValueFiltering demo by adding null and undefined safety checks throughout event handlers and DOM interactions. Changes include initializing refs to null, adding guards in event handlers, and using optional chaining to prevent runtime errors when elements or event targets are absent. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-core/src/demos/examples/TextInputGroup/AttributeValueFiltering.tsx (1)
193-203: LGTM! Robust event handling.The optional chaining and defensive text extraction prevent errors when the event or target is missing. The fallback to empty string and optional chaining on lines 201-202 make the function resilient.
Optional: Guard against empty text selection
For extra safety, you could bail out early if no text was extracted:
const onSelect = (event: React.MouseEvent<Element, MouseEvent> | undefined, _itemId: string | number) => { const selectedText = (event?.target as HTMLElement | undefined)?.innerText.trim() || ''; + + if (!selectedText) { + return; + } if (selectedKey.length) { selectValue(selectedText); } else { selectKey(selectedText); } event?.stopPropagation(); textInputGroupRef.current?.querySelector('input')?.focus(); };This prevents adding malformed chips if somehow an empty element is selected, though normal usage shouldn't trigger this.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/demos/examples/TextInputGroup/AttributeValueFiltering.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
🔇 Additional comments (6)
packages/react-core/src/demos/examples/TextInputGroup/AttributeValueFiltering.tsx (6)
43-44: LGTM! Correct ref initialization.Initializing refs with
nullinstead ofundefinedis the standard React pattern and improves type safety.
110-110: LGTM! Correct dependency array.Adding
menuItemsTextandselectedKeyto the dependency array is correct, as the effect uses both values. This ensures the menu updates properly when an attribute or value is selected.
131-145: LGTM! Improved Enter key handling.Extracting
firstResultfrommenuItems[2](the first menu item after heading and divider) and guarding against its absence makes the function more robust. This prevents errors when Enter is pressed with an empty menu.
155-155: LGTM! Correct event handling for colon key.Passing the event to
handleColonallows it to properly callpreventDefault()when auto-selecting a key, preventing the colon from being added to the input.Also applies to: 183-183
206-214: LGTM! Defensive click handling.Making the event parameter possibly undefined and using optional chaining throughout makes the function robust against missing events or targets.
119-129: LGTM! Core fix for the TypeError.The guard on lines 122-124 prevents calling
setMenuItemsTextwithundefined. At line 72,menuItemsText.filter()would throw a TypeError ifmenuItemsTextwas undefined. This guard prevents that by returning early when an invalid key is passed toselectKey, preserving the previous valid state.
|
Preview: https://pf-react-pr-12207.surge.sh A11y report: https://pf-react-pr-12207-a11y.surge.sh |
Problem
Changes
Closes #12206
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.