Skip to content

RN Turbo/Fabric migration #88

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

Merged
merged 34 commits into from
Apr 1, 2025
Merged

RN Turbo/Fabric migration #88

merged 34 commits into from
Apr 1, 2025

Conversation

ian-wd
Copy link

@ian-wd ian-wd commented Mar 7, 2025

Pull Request: react-native-sketch-canvas

Description

Turbo/Fabric View migration

Type of Change

  • New Plugin
  • Feature Addition
  • Bug Fix
  • Performance Improvement
  • Code Refactoring
  • Documentation Update
  • Other (please describe):

Testing Performed

  • Unit Tests
  • Integration Tests
  • Component Tests
  • Manual Testing
  • Edge Cases Verified

Test Results

image

Breaking Changes

  • Bumped minimum RN version to v0.76.7
  • New Architecture must be enabled

@iBotPeaches
Copy link
Member

@nemanjar7 - Can you pull this sample out down and give it a shot locally? Want to have someone else test to be sure we understand this movement to New Arch

@nemanjar7
Copy link

Screenshot 2025-03-12 at 1 39 56 PM

Are these logs supposed to be here?
Screenshot 2025-03-12 at 1 34 01 PM

Android:

android1.mov
android2.mov

iOS:

ios1.mov
ios2.mov

Seems fine to me, the only thing I noticed is that if we don't clear the screen on one canvas, it tends to transfer those changes to the new screen. Not sure if the app is set up that way since it's just example app.

@iBotPeaches
Copy link
Member

The logs are intentional since just a sample app to see what's it's doing. The drawing persisting seems odd - I'd like @ian-wd to comment on that.

@ian-wd
Copy link
Author

ian-wd commented Mar 14, 2025

The logs are intentional since just a sample app to see what's it's doing. The drawing persisting seems odd - I'd like @ian-wd to comment on that.

ill check this weekend, I haven't encountered that I'm mostly running this on android

@ian-wd
Copy link
Author

ian-wd commented Mar 17, 2025

Fixed iOS fabric view/sketchcanvas cleanup

ios-cleanup-fix.mp4

@ian-wd ian-wd marked this pull request as ready for review March 24, 2025 11:11
@ian-wd ian-wd changed the title [WIP] RN Turbo/Fabric migration RN Turbo/Fabric migration Mar 24, 2025
@ian-wd ian-wd requested a review from iBotPeaches March 26, 2025 03:58
@iBotPeaches
Copy link
Member

@nemanjar7 - May you confirm your issues you identified with iOS clearing is good? I think this is ready to merge if so.

@nemanjar7
Copy link

Okay, few findings:

  • The previous issues with screens not clearing are corrected
  • I noticed some other issues, for examples 6 and 7 (Video was too long so I only updated the issues with ex6 and ex7). When you get in the example 6 first time it shows these different kinds of text, so I just scribbled something and closed the screen, but when you get back on it, it shows blank screen. Also, not in the video, but changing text at the top, modifies only text on the left side. This could be though me not fully understanding the functionality of this particular screen.
  • Last screen has similar issue in a sense where returning to the screen after you just got out of it (and not checking any other screen) will show different UI.
  • If you switch screens (examples) however, everything is reset properly besides example 7. That one is very inconsistent since sometimes it shows 2 canvases, sometimes 1, and then sometimes nothing.
ex6-ex7.mov

Also I want to include a possibility that I maybe didn't set up something right, since when I tried to run the app from xcode is got the following error below(no matter what I did, I couldn't make it work like that). However, after running it from vscode with npm run ios it launched with no issues.

Screenshot 2025-03-31 at 3 54 58 PM

@nemanjar7
Copy link

nemanjar7 commented Mar 31, 2025

This is the other things I talked about. So if you have time to test it out maybe to confirm it's not just my local env? @iBotPeaches

extra.mov

@ian-wd
Copy link
Author

ian-wd commented Apr 1, 2025

This is the other things I talked about. So if you have time to test it out maybe to confirm it's not just my local env? @iBotPeaches

extra.mov

yeah this looks like regression @nemanjar7, probably related to the view cleanup

@nemanjar7
Copy link

I believe it's g2g now. @iBotPeaches

ex6ex7.mov

nemanjar7
nemanjar7 previously approved these changes Apr 1, 2025
@iBotPeaches iBotPeaches merged commit 66c1880 into master Apr 1, 2025
1 check passed
@iBotPeaches iBotPeaches deleted the fabric branch April 1, 2025 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants