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: adding back bounds #2015

Merged
merged 6 commits into from
Jul 26, 2022
Merged

feat: adding back bounds #2015

merged 6 commits into from
Jul 26, 2022

Conversation

Andarius
Copy link
Contributor

@Andarius Andarius commented Jun 30, 2022 •
edited
Loading

Description

  • Adding bounds to <MapboxGL.Camera />.
  • Fix for padding with bounds.
  • Adding None to animationMode.
  • Fix zoom/scroll/rotate/logo enabled/disabled

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I updated the documentation with running yarn generate in the root folder
  • I mentioned this change in CHANGELOG.md
  • I updated the typings files (index.d.ts)
  • I added/ updated a sample (/example)

@Andarius Andarius marked this pull request as ready for review June 30, 2022 16:35
@Andarius
Copy link
Contributor Author

I'm doing some testing, and it appears that when rendering more than 2 maps, the zoom cannot be computed
E/Mapbox: [maps-core]: {obitrain.obiapp}[General]: Unable to calculate appropriate zoom level for bounds. Vertical or horizontal padding is greater than map's height or width.
Not sure why, maybe related to #1057 ?

@Andarius
Copy link
Contributor Author

Andarius commented Jul 1, 2022 •
edited
Loading

m doing some testing, and it appears that when rendering more than 2 maps, the zoom cannot be computed
E/Mapbox: [maps-core]: {obitrain.obiapp}[General]: Unable to calculate appropriate zoom level for bounds. Vertical or horizontal padding is greater than map's height or width.
Not sure why, maybe related to #1057 ?

I think in fact that the width / height of the camera is not correctly set if out of the screen. If I reduce the size of the map, It works for the number of maps that are displayed, not the one hidden.

mfazekas
mfazekas reviewed Jul 5, 2022
View reviewed changes
javascript/components/Camera.js
@@ -616,6 +616,7 @@ class Camera extends React.Component {
case Camera.Mode.Linear:
return MapboxGL.CameraModes.Linear;
case Camera.Mode.None:
return MapboxGL.CameraModes.None;
case Camera.Mode.Move:
return MapboxGL.CameraModes.Move;
Copy link
Contributor

Choose a reason for hiding this comment

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

None is same as Move, but it's a bit of a mess as most implementation exports MapboxGL.CameraModes.None, but iOS on v10 only export Move

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do you disable the animation on iOS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Move just jumps without animation.

#1838 (comment)

@felixgourdeau
Copy link
Contributor

felixgourdeau commented Jul 24, 2022 •
edited
Loading

@mfazekas @Andarius should we merge this PR for now, and fix later the issue with more than 2 maps?

@mfazekas
Copy link
Contributor

@mfazekas @Andarius should we merge this PR for now, and fix later the issue with more than 2 maps?

Yes sorry, let's merge this. Please update code so it can be merged.

mfazekas
mfazekas requested changes Jul 26, 2022
View reviewed changes
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.

I think logoEnabled, and compassEnabled changes needs revert

android/rctmgl/src/main/java-v10/com/mapbox/rctmgl/components/mapview/RCTMGLMapViewManager.kt Show resolved Hide resolved
android/rctmgl/src/main/java-v10/com/mapbox/rctmgl/components/mapview/RCTMGLMapViewManager.kt Outdated Show resolved Hide resolved
@mfazekas mfazekas merged commit f1a1623 into rnmapbox:main Jul 26, 2022
@mfazekas mfazekas mentioned this pull request Jul 26, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixgourdeau felixgourdeau felixgourdeau approved these changes

@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.
3 participants
@Andarius @felixgourdeau @mfazekas