Add support for date range picker on Android #956
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This implementation models how the Material date picker was implemented. However, the range picker returns two values and accepts two values for the start and end of the range.
sidorchukandrew
force-pushed
the
range-picker
branch
from
2001f26 to
079237c
Compare
Since this is a new module, I think we should only support the imperative API the same way we encourage developers to use the imperative API for the date and time pickers.
sidorchukandrew
force-pushed
the
range-picker
branch
from
079237c to
7d1c6e7
Compare
vonovak
requested changes
Mar 4, 2025
|
|
||
| class MaterialRangePickerModule(reactContext: ReactApplicationContext): NativeModuleMaterialRangePickerSpec(reactContext) { | ||
| companion object { | ||
| const val NAME = "RNCMaterialRangePicker" |
There was a problem hiding this comment.
If I'm not mistaken, NativeModuleMaterialRangePickerSpec generated by codegen already should contain the name, and so specifying it here should not be needed. also override fun getName should be in the generated code.
if not, it's fine
Comment on lines
+27
to
+34
| val activity = currentActivity as FragmentActivity? | ||
| if (activity == null) { | ||
| promise.reject( | ||
| RNConstants.ERROR_NO_ACTIVITY, | ||
| "Tried to open a MaterialRangePicker dialog while not attached to an Activity" | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Suggested change
| val activity = currentActivity as FragmentActivity? | |
| if (activity == null) { | |
| promise.reject( | |
| RNConstants.ERROR_NO_ACTIVITY, | |
| "Tried to open a MaterialRangePicker dialog while not attached to an Activity" | |
| ) | |
| return | |
| } | |
| val activity = currentActivity as FragmentActivity? ?: run { | |
| return promise.reject( | |
| RNConstants.ERROR_NO_ACTIVITY, | |
| "Tried to open a MaterialRangePicker dialog while not attached to an Activity" | |
| ) | |
| } |
this is more kotlin-style; just a suggestion. I'm not sure how it'll work with the cast though.
|
|
||
| if (args.containsKey(RNConstants.ARG_START_TIMESTAMP)) { | ||
| // override "value" so we can use the same constructor from RNDate | ||
| args.putLong(RNConstants.ARG_VALUE, args.getLong((RNConstants.ARG_START_TIMESTAMP))) |
There was a problem hiding this comment.
rather than mutating args in order to use some constructor of RNDate, I'd add another constructor to RNDate which accepts Long value
Comment on lines
+67
to
+70
| val title = args.getString(RNConstants.ARG_TITLE) | ||
| if (!title.isNullOrEmpty()) { | ||
| builder.setTitleText(args.getString(RNConstants.ARG_TITLE)) | ||
| } |
There was a problem hiding this comment.
Suggested change
| val title = args.getString(RNConstants.ARG_TITLE) | |
| if (!title.isNullOrEmpty()) { | |
| builder.setTitleText(args.getString(RNConstants.ARG_TITLE)) | |
| } | |
| args.getString(RNConstants.ARG_TITLE)?.let { | |
| builder.setTitleText(it) | |
| } |
more kotlin-y. You can use this pattern more across this file pls, e.g in setButtons.
Comment on lines
+163
to
+171
| val result = WritableNativeMap() | ||
|
|
||
| result.putString("action", RNConstants.ACTION_RANGE_SET) | ||
| result.putDouble("startTimestamp", getStartTimestamp(selection)) | ||
| result.putDouble("endTimestamp", getEndTimestamp(selection)) | ||
| result.putDouble( | ||
| "utcOffset", | ||
| getStartTimestamp(selection) / 1000 / 60 | ||
| ) |
There was a problem hiding this comment.
Suggested change
| val result = WritableNativeMap() | |
| result.putString("action", RNConstants.ACTION_RANGE_SET) | |
| result.putDouble("startTimestamp", getStartTimestamp(selection)) | |
| result.putDouble("endTimestamp", getEndTimestamp(selection)) | |
| result.putDouble( | |
| "utcOffset", | |
| getStartTimestamp(selection) / 1000 / 60 | |
| ) | |
| val result = WritableNativeMap().apply { | |
| putString("action", RNConstants.ACTION_RANGE_SET) | |
| putDouble("startTimestamp", getStartTimestamp(selection)) | |
| putDouble("endTimestamp", getEndTimestamp(selection)) | |
| putDouble( | |
| "utcOffset", | |
| getStartTimestamp(selection) / 1000 / 60 | |
| ) | |
| } |
more koltin-y. You can use apply in many places throughout this file to save keystrokes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This adds support for Material 3's date range picker. I've added this as an imperative API similar to how the date and time pickers can be called imperatively on Android. Many of the props the new date Material date picker supports are supported here as well. There are some small differences in the following props:
valueandonChange.valuemust be aRangeobject that looks like{start: Date?, end: Date?}. These are optional and thevalueprop itself is optional. Leaving it undefined will open the picker with a clean slate.onChangereturns astartTimestampandendTimestampin thenativeEventpayload and returns aRangeobject as the second parameter.See the README for a screenshot.
Test Plan
What's required for testing (prerequisites)?
What are the steps to reproduce (after prerequisites)?
Compatibility
Checklist
README.mdexample/App.js)