Skip to content

feat(v10, ios) Custom compass image#2352

Merged
mfazekas merged 14 commits intornmapbox:mainfrom
TruckMap:feat/compass
Nov 10, 2022
Merged

feat(v10, ios) Custom compass image#2352
mfazekas merged 14 commits intornmapbox:mainfrom
TruckMap:feat/compass

Conversation

@naftalibeder
Copy link
Collaborator

@naftalibeder naftalibeder commented Oct 28, 2022

This enables a custom image to be displayed in place of the default compass.

compass.mov

@naftalibeder
Copy link
Collaborator Author

@mfazekas Android actually has a lot of problems with ornaments - see the video below. Do you want me to have a go at fixing these, or do you think I should make this PR exclusively about the iOS compass image?

android-compass.mov

Copy link
Contributor

@mfazekas mfazekas left a comment

Choose a reason for hiding this comment

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

Thanks, much, looks great!

This is iOS only right?
Do you plan to implement it on android as well?

@mfazekas
Copy link
Contributor

@mfazekas Android actually has a lot of problems with ornaments - see the video below. Do you want me to have a go at fixing these, or do you think I should make this PR exclusively about the iOS compass image?

android-compass.mov

I'd prefer fixing in a separate issue, but if it's more convenient it can be fixed here as well.

@naftalibeder
Copy link
Collaborator Author

Do you plan to implement it on android as well?

That was my goal, but I'm actually having trouble even getting the compass to show up on Android at all. The scale bar looks weird too:

image

Do you see something different? This is in other example files to, not just the Ornaments one.

@mfazekas
Copy link
Contributor

Do you plan to implement it on android as well?

That was my goal, but I'm actually having trouble even getting the compass to show up on Android at all. The scale bar looks weird too:

image

Do you see something different? This is in other example files to, not just the Ornaments one.

No I haven't looked into android ornaments much yet.
Sure we can do in other PR too.

@mysport12
Copy link
Contributor

@naftalibeder @mfazekas The weird scale bar issue on Android is tied to a map view dimensions race condition. When the scale bar width is calculated on native I don't believe it has the final map view width and I didn't have enough time to look into how we could pass down the final value after the map is laid out.

Test case: Rotate the app so the map layout is recalculated for landscape mode and notice the scale bar looks correct, then rotate back.

@naftalibeder
Copy link
Collaborator Author

naftalibeder commented Nov 2, 2022

@mfazekas Using the compass image property produces some challenges - for instance, sizing the compass, adding shadow that doesn't rotate, not to mention any animation is impossible. This is something you could easily do in a native app, but the React Native bridge bottleneck makes it unworkable.

I did a quick proof of concept that shows how we could provide a <MapCompass /> component that does nothing except render children and rotate according to the map rotation, as shown.

Screen.Recording.2022-11-02.at.4.08.45.PM.mov

Usage would be something like:

<MyCustomWrapper> // does not rotate
  <MapCompass>
    <MyCustomCompassIcon /> // rotates according to the map
  </MapCompass>
</MyCustomWrapper>

How would you feel about that?

@mfazekas
Copy link
Contributor

mfazekas commented Nov 7, 2022

@mfazekas Using the compass image property produces some challenges - for instance, sizing the compass, adding shadow that doesn't rotate, not to mention any animation is impossible. This is something you could easily do in a native app, but the React Native bridge bottleneck makes it unworkable.

I did a quick proof of concept that shows how we could provide a <MapCompass /> component that does nothing except render children and rotate according to the map rotation, as shown.

Screen.Recording.2022-11-02.at.4.08.45.PM.mov

Usage would be something like:

<MyCustomWrapper> // does not rotate
  <MapCompass>
    <MyCustomCompassIcon /> // rotates according to the map
  </MapCompass>
</MyCustomWrapper>

How would you feel about that?

Looks acceptable for me. What about just providing a callback for rotation changes, so users can display animate the way they prefer?

@naftalibeder
Copy link
Collaborator Author

What about just providing a callback for rotation changes, so users can display animate the way they prefer?

Maybe I'm misunderstanding, but if you're talking about a JavaScript callback, the issue is that sending that many events over the bridge that quickly is very inefficient and causes a lot of lag.

Since a component like this only needs to rotate according to the camera bearing, all the rotation animation logic can exist on the native side, while allowing React Native to handle the view.

(And there already is a callback for getting the current bearing, regionIsChanging.)

@naftalibeder naftalibeder changed the title Custom compass image fix(v10, iOS) Custom compass image Nov 8, 2022
@naftalibeder naftalibeder changed the title fix(v10, iOS) Custom compass image fix(v10, ios) Custom compass image Nov 8, 2022
@naftalibeder naftalibeder changed the title fix(v10, ios) Custom compass image feat(v10, ios) Custom compass image Nov 8, 2022
@naftalibeder
Copy link
Collaborator Author

@mfazekas are you open to considering this an iOS-only feature and merging it? I can't even get the compass to show up on Android :/ - I plan on looking into it in the future but I can't right now.

Then I would open the idea for a <Compass /> component as a new issue we can discuss.

@mfazekas
Copy link
Contributor

mfazekas commented Nov 9, 2022

@mfazekas are you open to considering this an iOS-only feature and merging it? I can't even get the compass to show up on Android :/ - I plan on looking into it in the future but I can't right now.

Sure that's fine, just add that the docs that it's iOS v10 only.

@naftalibeder
Copy link
Collaborator Author

@mfazekas good to go - ok?

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.

3 participants