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

Add allowUpdates prop on Camera #1619

Merged
merged 5 commits into from
Nov 15, 2021
Merged

Conversation

naftalibeder
Copy link
Collaborator

Description

Adds an optional allowUpdates boolean prop to Camera which, if false, causes the camera to return early before communicating with the native camera module.

This is intended for situations where the map is not visible (including if the app is in the background), and it would be preferable to avoid moving the camera - and therefore fetching new tiles, or causing the markers on the map to change - without actually hiding the map, which can cause undesirable behavior when the map reappears.

The only reasonable alternative is to "freeze" all props before sending them to Camera, which adds a bit more complicated logic to the enclosing app.

Checklist

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

Screenshot OR Video

TODO

@naftalibeder
Copy link
Collaborator Author

@ferdicus What do you think about this? We've been working around this for a pretty long time, and I think this would greatly simplify how an app needs to handle this situation. I know this prop doesn't have a corresponding config in the native modules, but it is specific to React logic (e.g. you can't "early return" in declarative code).

Copy link
Member

@ferdicus ferdicus left a comment

Choose a reason for hiding this comment

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

Looks good to me, optional after all 👍🏿

Do you have an example of how/ when you're using this?

@naftalibeder
Copy link
Collaborator Author

Yep, we do:

<Camera
  ...
  allowUpdates={appStateIsActive || pictureInPicture}
/>

(The picture-in-picture conditional is for Android.)

@ferdicus
Copy link
Member

thanks

@ferdicus ferdicus merged commit 79e82e0 into rnmapbox:master Nov 15, 2021
@ferdicus
Copy link
Member

@naftalibeder, this is failing tests after merge (sorry, didn't catch it earlier - forked PRs don't run actions 😞 ):

@naftalibeder
Copy link
Collaborator Author

Sorry about that - fixed here.

Maybe the default PR checklist should include yarn run unittest?

@ferdicus
Copy link
Member

Sorry about that - fixed here.

Maybe the default PR checklist should include yarn run unittest?

Good thing you mentioned it - I've added an additional todo in #1631 (sorry.... 😞 )

@naftalibeder naftalibeder deleted the feat/camera-updates branch February 21, 2024 15:46
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