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

MarkerView has allowOverlapWithPuck #3212

Merged

Conversation

MichaelDanielTom
Copy link
Contributor

Feat: MarkerView has allowOverlapWithPuck, MarkerView example updated and converted to TypeScript.

Haven't tested completely on Android yet but the example works well on iOS. This is my first PR that has autogenerated stuff from the pre-commit hooks, so lmk if there's anything that didn't generate properly or if anything else needs to be run.

@mfazekas
Copy link
Contributor

@MichaelDanielTom thanks much, look good to me!

@@ -0,0 +1,108 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

There is now a type:check error on examples - see CI.

FWIW examples are not meant to be all typescript, but it's fine to convert some to ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just fixed - won't convert any other examples to typescript if they aren't already in the future.

Also for making small fixes like this after review, would you rather I create new commits with the fixes, or amend the original commit(s) and force push?

@mfazekas
Copy link
Contributor

mfazekas commented Dec 1, 2023

@MichaelDanielTom pls check build errors on Android:

> Task :rnmapbox_maps:compileDebugKotlin FAILED
e: file:///home/runner/work/maps/maps/android/src/main/java/com/rnmapbox/rnmbx/components/annotation/RNMBXMarkerView.kt:183:13 Unresolved reference: allowOverlapWithPuck
e: file:///home/runner/work/maps/maps/android/src/main/java/com/rnmapbox/rnmbx/components/annotation/RNMBXMarkerViewManager.kt:54:44 Unresolved reference: allowOverlapWithPuck
e: file:///home/runner/work/maps/maps/android/src/main/java/com/rnmapbox/rnmbx/components/images/RNMBXImagesManager.kt:205:16 Unresolved reference: eventMapOf

Last one was issue on main, so you need to merge/rabase to origin/main

@MichaelDanielTom
Copy link
Contributor Author

@mfazekas Do you have any preferred workflow for implementing and testing new code besides building and running each of the example projects and having the pre-commit hooks run? Also should we be creating equivalent examples in both example and fabricexample for new features to test? Right now I'm testing this change by:

  1. Writing/modifying an example in example/ e.g. example/src/examples/Annotations/MarkerView.js.
  2. Manually testing on android and ios with yarn android and yarn ios respectively from example/.
  3. Writing/modifying an example in fabricexample/ e.g. fabricexample/src/examples/Annotations/MarkerView.js.
  4. Manually testing on android and ios with yarn android and yarn ios respectively from fabricexample/.
  5. Commit, push, and wait to see if github actions pass.

Are there any other steps I'm forgetting or should be doing?

@mfazekas
Copy link
Contributor

mfazekas commented Dec 1, 2023

@mfazekas Do you have any preferred workflow for implementing and testing new code besides building and running each of the example projects and having the pre-commit hooks run? Also should we be creating equivalent examples in both example and fabricexample for new features to test? Right now I'm testing this change by:

  1. Writing/modifying an example in example/ e.g. example/src/examples/Annotations/MarkerView.js.
  2. Manually testing on android and ios with yarn android and yarn ios respectively from example/.
  3. Writing/modifying an example in fabricexample/ e.g. fabricexample/src/examples/Annotations/MarkerView.js.
  4. Manually testing on android and ios with yarn android and yarn ios respectively from fabricexample/.
  5. Commit, push, and wait to see if github actions pass.

Are there any other steps I'm forgetting or should be doing?

Thanks for bringing this up. We should document this in CONTRUBUTING.md I think.

So some notes:

I've removed commit hooks as they are anoying. We have a lot of code generation going on, yarn generate will update generated code, and will show diffs if they are different.

Also you want to run:

  • yarn test to run unittest
  • `yarn lint' to run eslint
  • yarn type:check to run typescript checks
  • cd example ; yarn type:check to run typescript check on example directory
  1. Writing/modifying an example in example/ e.g. example/src/examples/Annotations/MarkerView.js.

Note that examples has two purpose, testing that stuff works and demonstrating how to use it. We might need separate example for showing a simple way to use it, and a more complex example so we can test all the features.

  1. Manually testing on android and ios with yarn android and yarn ios respectively from example/.

Yep.

  • To test V11 on iOS, I usually do:
    RNMBX11=1 pod update MapboxMaps
  • To test fabric on iOS I usually do:
    RCT_NEW_ARCH_ENABLED=1 pod update MapboxMaps

(Also fabric could be tested combined with RNMBX11=1 )

They might work with RCT_NEW_ARCH_ENABLED=1 yarn react-native run-ios and RNMBX11=1 yarn react-native run-ios. But I haven't tested.

On android I usually change grade.properties => newArchEnabled and RNMBX11 flags.

This could be set with ORG_GRADLE_PROJECT_newArchEnabled=1 and ORG_GRADLE_PROJECT_RNMBX11=1 but I haven't tested that either.

  1. Writing/modifying an example in fabricexample/ e.g. fabricexample/src/examples/Annotations/MarkerView.js.

I'm planning to remove fabric example as it's a code duplicate and the only advantage is that you can try both fabric and old arch without rebuild.

@mfazekas
Copy link
Contributor

mfazekas commented Dec 4, 2023

See error on CI for iOS I think you need to add v11 specific code to

#If RNMBX_11
...
#endif

@mfazekas mfazekas merged commit f6ee1b0 into rnmapbox:main Dec 5, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants