-
-
Notifications
You must be signed in to change notification settings - Fork 453
fix(ios): internal picker state persisting with recycling #1026
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
fix(ios): internal picker state persisting with recycling #1026
Conversation
ios/RNDateTimePicker.m
Outdated
| { | ||
| self.minimumDate = nil; | ||
| self.maximumDate = nil; | ||
| self.date = [NSDate date]; |
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.
can't set this to nil - this appears to be the default when you instantiate a UIDatePicker
|
Thank you for this @sterlingwes , can't wait for this to be merged! :) |
vonovak
left a 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.
thanks for the PR. Could this be resolved by disabling recycling altogether? The benefit for date pickers is minimal and there's a lot of internal state that could mess things up.
see https://blog.swmansion.com/react-natives-new-architecture-the-tricky-parts-1-2-bb0c16950f2d
+ (BOOL)shouldBeRecycled
{
return NO;
}
|
yup good call @vonovak , simpler is better here for sure did the same before / after test and can confirm this fixes as shown above |
48e89d2
into
react-native-datetimepicker:master
|
Thank you @vonovak and @sterlingwes . When could I expect a new release? :) |
|
@Junveloper momentarily - when this MR lands: #1027 Thanks for sharing the reproduction case! And to vonovak for the quick review 🙏🏻 |
|
Thank you, you guys are legends! |
Summary
Follow-up to the min/max constraint fix in #1009. There was a pre-existing issue when the component gets unmounted: the native component gets recycled for future reuse but keeps its picker instance properties along with their state.
This likely would have also fixed the earlier issue but I think those changes are worth keeping as they make it clearer how the UIDatePicker constraints operate.
Test Plan
Video showing the crash case:
pickerbug.mp4
Two breakpoints here:
Video showing the fix:
pickerbugfixed.mp4
Breakpoints show that the min/max date constraints are cleared
What's required for testing (prerequisites)?
I copied over this component from the reproduction app (provided by @Junveloper in the prior PR). You can reference this branch for the changes required: sterlingwes#1
What are the steps to reproduce (after prerequisites)?
Compatibility
Checklist
README.mdexample/App.js)