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.

Sign up for GitHub

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

Jump to bottom

feat: migrate components to new arch #3136

Merged
merged 6 commits into from
Oct 26, 2023
Merged

feat: migrate components to new arch #3136

merged 6 commits into from
Oct 26, 2023

Conversation

WoLewicki
Copy link
Contributor

@WoLewicki WoLewicki commented Oct 24, 2023 •
edited
Loading

PR adding New Architecture support to the library 🎉

We at Software Mansion have been working on improving support for the new architecture for quite a while now. If you need help with anything related to New Architecture, like:

or you just want to ask any questions, hit us up on [email protected]

Notes

PR migrating rest of the components to new arch and cleaning some code.

@WoLewicki
Copy link
Contributor Author

@mfazekas I have 2 questions here:

  1. Is refresh used in Image component? It is not implemented on iOS from what I can see.
  2. There are some props on Android with a TODO in them, I am not sure what to do with them, so it would be nice to give an answer for it 🚀

mfazekas
mfazekas reviewed Oct 24, 2023
View reviewed changes
android/src/main/java/com/rnmapbox/rnmbx/components/styles/terrain/RNMBXTerrainManager.kt Outdated
fun setId(layer: RNMBXTerrain, id: String?) {
layer.iD = id
}
// TODO: it is not present in props so should we add it?
Copy link
Contributor

Choose a reason for hiding this comment

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

id is not needed for terrain it's a singleton

@mfazekas
Copy link
Contributor

@mfazekas I have 2 questions here:

  1. Is refresh used in Image component? It is not implemented on iOS from what I can see.

Yes refresh is needed, just did not get to implement that on iOS yet. Leave it as no-op on iOS. We need that if the child view of Image component has an Image for example that doesn't draw immediately, and we'll need to call refresh when the image has loaded.

  1. There are some props on Android with a TODO in them, I am not sure what to do with them, so it would be nice to give an answer for it 🚀

@WoLewicki WoLewicki changed the title feat: migrate js code of components feat: migrate components to new arch Oct 25, 2023
@WoLewicki
Copy link
Contributor Author

Ok I pushed the changes with Image module added. Could you check if it works correctly, and, if so, are we ready to merge this one too 🚀 ?

@mfazekas mfazekas merged commit d2c55d7 into main Oct 26, 2023
8 of 9 checks passed
@mfazekas mfazekas deleted the @wolewicki/migrate-components branch October 26, 2023 11:10
@mfazekas
Copy link
Contributor

Looks good to me, v11 on iOS needs to be fixed, but that can be done later.

mfazekas added a commit that referenced this pull request Oct 27, 2023
@mfazekas
fix(ios,v11): some RNMBX_11 were destroyed during the merge of #3136
a775fc3
mfazekas added a commit that referenced this pull request Oct 27, 2023
@mfazekas
refactor: remove RNMBPXAndroidTextureMapView, and lateinit MapView on… (
f5ae784 
#3138)

* refactor: remove RNMBPXAndroidTextureMapView, and lateinit MapView on android

* fix: use_frameworks - use rnmapbox_maps-Swift.pre.h

* fix: correct ts types of codgen on RNMBXMapView

* fix(ios,v11): some RNMBX_11 were destroyed during the merge of #3136
@WoLewicki WoLewicki mentioned this pull request Aug 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfazekas mfazekas mfazekas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@WoLewicki @mfazekas