Repository navigation
Enhance DrumPicker with custom item rendering and new Row component - #16
Conversation
Introduced a `renderItem` prop to the DrumPicker component, allowing users to customize the rendering of picker items with their own React components. This enhancement maintains native scroll physics and snap behavior while providing flexibility in item presentation. Updated README and types to reflect this new feature.
Updated the withVirtualized HOC to support a custom renderItem prop, allowing for more flexible item rendering in the VirtualizedDrumPicker. Added a new test case to verify that the renderItem function correctly updates the isSelected state based on the real index. Updated README to include performance recommendations for complex rows.
Introduced a new Row component for optimized rendering of picker items using React.memo. Updated the README to include the new Row component example and performance recommendations. Enhanced the DrumPicker test to verify the functionality of the renderItem prop, ensuring it correctly updates the selected state during interactions.
… image Removed the outdated `DateDrumPicker` preview image and added multiple new demo images showcasing various picker functionalities, including basic, time, height/weight, controlled selection, and date picking. This enhances the documentation by providing clearer visual examples of the component's capabilities.
…tem handling Replaced local image paths in the README with direct URLs for better accessibility. Updated the DrumPicker component to utilize a labels array for improved item handling, ensuring that the correct values are returned and clamped based on the labels instead of the original items. Added a new test case to verify the functionality of rendering items with object mappings.
… directory to package.json Changed image paths in the README to reference local files instead of external URLs for better accessibility. Added the 'func-gif' directory to package.json to ensure it is included in the package distribution.
|
Warning Review limit reached
More reviews will be available in 48 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds support for custom item rendering via a new ChangesCustom item rendering with renderItem prop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/withVirtualized.tsx (1)
236-243:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUse label-based values in virtualized mode (not raw
items[...]).These paths still emit/resolve values using raw items. With non-string items,
onChange/onValueChangingcan return objects andscrollToValuecan fail, which conflicts withDrumPickerChangeEvent.valueand non-virtualized behavior.Suggested direction
+const labels = useMemo(() => items.map((item) => getItemLabel(item)), [items]); - value: items[clamped] ?? '', + value: labels[clamped] ?? '', - const index = items.indexOf(value); + const index = labels.indexOf(value); - return items[parentIndexRef.current] ?? ''; + return labels[parentIndexRef.current] ?? ''; - value: items[realIdx] ?? event.nativeEvent.value, + value: labels[realIdx] ?? event.nativeEvent.value,Also pass
labels(notitems) intoresolveRealIndex(...)where label matching is required.Also applies to: 341-353, 383-389, 410-416
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/withVirtualized.tsx` around lines 236 - 243, The code currently emits raw items (items[...]) in virtualized mode; change these emissions and any index-resolution to use label-based values instead: when calling onChange/onValueChanging replace value: items[clamped] with value: labels[clamped] (or the corresponding label lookup) and when calling resolveRealIndex(...) pass labels (not items) so label matching is used; update the same pattern in the other locations referenced (around the blocks that call onValueChanging, onChange, scrollToValue, and resolveRealIndex) to ensure virtualized mode uses labels consistently and avoids returning non-string objects.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 31-35: Update the `[Unreleased]` comparison link so its base is
`v0.2.3` instead of including already released changes; locate the
`[Unreleased]` heading/link in CHANGELOG.md and change the compare URL/anchor to
start from `v0.2.3` (ensure subsequent released entries remain unchanged) so the
diff only shows changes after v0.2.3.
In `@README.md`:
- Around line 500-503: The props table incorrectly documents DrumPicker.items as
`string[]`; update the docs to reflect the generic contract by describing
`items` as a generic array (e.g., `T[]` or `Array<T>`) and note that consumers
can pass objects when using the component's generic type parameter; update the
`Prop`/`Type` column for `items` and adjust the `Description` to mention that
the shape is determined by the generic type `T` and mapping/render props (refer
to DrumPicker.items and any object-item examples in the README to ensure
consistency).
In `@src/DrumPicker.native.tsx`:
- Line 216: The exported DrumPicker currently erases generics by using
DrumPickerProps<any>; change the forwardRef signature to preserve the generic
type parameter so consumers retain type-safety: make DrumPicker a generic
forwardRef with a type parameter T (e.g., forwardRef<DrumPickerRef,
DrumPickerProps<T>>) or export a generically-typed wrapper/cast that returns a
React.ForwardRefExoticComponent<DrumPickerProps<T> &
React.RefAttributes<DrumPickerRef>>, ensuring DrumPickerProps<T> and
renderItem/item typings are preserved.
In `@src/DrumPickerWithRenderItem.tsx`:
- Around line 43-45: currentIndex and changingIndex are only set from
selectedIndex at initialization, so when selectedIndex changes externally the
overlay (changingIndex) can get out of sync; update both state values whenever
selectedIndex changes by adding an effect that sets
setCurrentIndex(selectedIndex) and setChangingIndex(selectedIndex) inside a
useEffect that depends on selectedIndex (so liveIndex = onValueChanging != null
? changingIndex : currentIndex stays correct). Ensure the effect targets the
existing state setters currentIndex/changingIndex and does not run on every
render unnecessarily.
---
Outside diff comments:
In `@src/withVirtualized.tsx`:
- Around line 236-243: The code currently emits raw items (items[...]) in
virtualized mode; change these emissions and any index-resolution to use
label-based values instead: when calling onChange/onValueChanging replace value:
items[clamped] with value: labels[clamped] (or the corresponding label lookup)
and when calling resolveRealIndex(...) pass labels (not items) so label matching
is used; update the same pattern in the other locations referenced (around the
blocks that call onValueChanging, onChange, scrollToValue, and resolveRealIndex)
to ensure virtualized mode uses labels consistently and avoids returning
non-string objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0d0c389e-73a0-4130-9c1a-38f2b8e56b3d
⛔ Files ignored due to path filters (8)
func-gif/Booking.gifis excluded by!**/*.giffunc-gif/Controller.gifis excluded by!**/*.giffunc-gif/Date.gifis excluded by!**/*.giffunc-gif/Debounce.gifis excluded by!**/*.giffunc-gif/Large-List.gifis excluded by!**/*.giffunc-gif/basic.gifis excluded by!**/*.giffunc-gif/heigh-weight.gifis excluded by!**/*.giffunc-gif/time.gifis excluded by!**/*.gif
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mdpackage.jsonsrc/DrumPicker.native.tsxsrc/DrumPicker.tsxsrc/DrumPickerWithRenderItem.tsxsrc/__tests__/DrumPicker.web.test.tsxsrc/__tests__/DrumPickerRenderItem.test.tsxsrc/__tests__/withVirtualized.test.tsxsrc/index.tsxsrc/itemLabel.tssrc/types.tssrc/withVirtualized.tsx
| ); | ||
| }); | ||
|
|
||
| export const DrumPicker = forwardRef<DrumPickerRef, DrumPickerProps<any>>( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Preserve generic item typing on exported DrumPicker instead of any.
Line 216 hard-codes DrumPickerProps<any>, which drops type-safety for items and renderItem payloads in consumer code. Please keep the forwarded component generic (or cast to a generic callable signature) so the new DrumPickerProps<T> contract is actually enforced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/DrumPicker.native.tsx` at line 216, The exported DrumPicker currently
erases generics by using DrumPickerProps<any>; change the forwardRef signature
to preserve the generic type parameter so consumers retain type-safety: make
DrumPicker a generic forwardRef with a type parameter T (e.g.,
forwardRef<DrumPickerRef, DrumPickerProps<T>>) or export a generically-typed
wrapper/cast that returns a React.ForwardRefExoticComponent<DrumPickerProps<T> &
React.RefAttributes<DrumPickerRef>>, ensuring DrumPickerProps<T> and
renderItem/item typings are preserved.
…ed display name Updated the DrumPicker component to utilize generic typing for better type safety and flexibility. Introduced a new implementation for the DrumPicker, ensuring the display name is correctly set. This refactor improves the overall structure and maintainability of the code.
1 participant
Summary
What does this PR change and why?
Type of change
Checklist
yarn lint,yarn build,yarn typecheck, andyarn testfrom the repo rootyarn.lockifpackage.jsondependencies changedsrc/,android/, orios/(example app only)CI
PRs to
mainrun the full CI workflow automatically. New tests in standard locations are picked up without editingci.yml.Screenshots / recordings (UI changes)
If applicable, add before/after visuals.
Related issues
Fixes # (issue number)
Summary by CodeRabbit
New Features
renderItemprop to customize how items are rendered in DrumPicker.Documentation
Chores