Skip to content
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

Add Fabric support #657

Merged
merged 7 commits into from
Sep 27, 2022

Conversation

alfonsocj
Copy link
Contributor

@alfonsocj alfonsocj commented Sep 13, 2022

Summary

Adding Fabric support for iOS.

  • Setup codegen spec in flow
  • Update podspec file
  • Add codegenConfig in package.json
  • Ensure backwards compatibility
  • iOS Implementation, RNDateTimePickerComponentView
  • Custom ShadowNodes.cpp and ComponentDescriptors.cpp in fabric to pass the size of the UIDatePicker based on the ones generated by codegen.
  • Custom RNDateTimePickerState.cpp that stores the frameSize to update the shadow node size.
  • The approach to calculate the intrinsic size of the component is based on react-native-screens approach and the cpp example from @cortinico and @troZee.

Android is already compatible with Fabric as it doesn't use a native UI component.

Test Plan

yarn start
cd "example/ios" && RCT_NEW_ARCH_ENABLED=1 npx pod-install && cd -
yarn start:ios

If you want to go back to the old renderer (Paper),
remove ios/build, run pod-install without the RCT_NEW_ARCH_ENABLED=1 and build again

rm -r "example/ios/build"
cd "example/ios" && npx pod-install && cd -
yarn start:ios

What's required for testing (prerequisites)?

What are the steps to reproduce (after prerequisites)?

Compatibility

OS Implemented
iOS ✅ ❌
Android ✅ ❌

Checklist

  • I have tested this on a device and a simulator
  • I added the documentation in README.md
  • I updated the typed files (TS and Flow)
  • I added a sample use of the API in the example project (example/App.js)
  • I have added automated tests, either in JS or e2e tests, as applicable

@alfonsocj alfonsocj force-pushed the fabric-support branch 4 times, most recently from 3fa7ae3 to e41783f Compare September 15, 2022 15:02
@alfonsocj alfonsocj changed the title Fabric support Add Fabric support Sep 15, 2022
@alfonsocj alfonsocj force-pushed the fabric-support branch 5 times, most recently from 5b397d6 to e0cccc9 Compare September 15, 2022 16:08
@@ -109,19 +116,16 @@ export default function Picker({
return null;
}

const dates: DatePickerOptions = {value, maximumDate, minimumDate};
toMilliseconds(dates, 'value', 'minimumDate', 'maximumDate');
Copy link
Contributor Author

@alfonsocj alfonsocj Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's difficult to cast this since we are changing the value in place.
A better alternative is to have a dateToMilliseconds helper fn that returns a ?number

@alfonsocj alfonsocj marked this pull request as ready for review September 15, 2022 16:45
src/types.js Outdated Show resolved Hide resolved
src/datetimepicker.ios.js Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@vonovak
Copy link
Member

vonovak commented Sep 16, 2022

@alfonsocj can you pls rebase on master, there have been some new changes, thank you! 🙂

@alfonsocj
Copy link
Contributor Author

now it doesn't show the native element because the height and width are not being calculated 😅

@alfonsocj alfonsocj force-pushed the fabric-support branch 2 times, most recently from 8324155 to 000aad6 Compare September 21, 2022 20:40
@alfonsocj alfonsocj force-pushed the fabric-support branch 2 times, most recently from 1518405 to 841dab1 Compare September 22, 2022 10:12
@alfonsocj
Copy link
Contributor Author

@vonovak I think the PR is ready 🙏.
Let me know if the changes make sense.

Copy link
Member

@vonovak vonovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not run the code yet (will do later to test it out) but I have some comments:

Thank you for the hard work! :)

src/datetimepicker.ios.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
ios/fabric/RNDateTimePickerComponentView.mm Show resolved Hide resolved
ios/fabric/RNDateTimePickerComponentView.mm Outdated Show resolved Hide resolved
@alfonsocj
Copy link
Contributor Author

Thanks for taking a look. I solved all the issues you mentioned 🤞

@vonovak vonovak merged commit 8df590b into react-native-datetimepicker:master Sep 27, 2022
vonovak pushed a commit that referenced this pull request Sep 27, 2022
# [6.4.0](v6.3.5...v6.4.0) (2022-09-27)

### Features

* **ios:** add Fabric support ([#657](#657)) ([8df590b](8df590b))
@vonovak
Copy link
Member

vonovak commented Sep 27, 2022

🎉 This issue has been resolved in version 6.4.0 🎉

If this package helps you, consider sponsoring us! 🚀

@muskaanbakshi
Copy link

Has anyone faced this issue in running build after doing RCT_NEW_ARCH_ENABLED=1 npx pod-install?
I get this error: @react-native-community/datetimepicker/ios/fabric/cpp/react/renderer/components/RNDateTimePicker/ShadowNodes.h:12:10: fatal error: 'react/renderer/components/RNDateTimePickerCGen/EventEmitters.h' file not found

#826

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants