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

Allowing ability to set Padding as stop #1844

Closed
wants to merge 2 commits into from

Conversation

dorthwein
Copy link
Contributor

@dorthwein dorthwein commented Apr 16, 2022

Fixes issue where padding is not respected when using .setCamera()

Description

Fixes issue where padding not respected when used in Camera Stops

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • I formatted JS and TS files with running yarn lint:fix in the root folder
  • 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)

Screenshot OR Video

Fixes issue where padding is not respected when using .setCamera()
@naftalibeder
Copy link
Collaborator

@dorthwein When I attempt to run this, I do see the padding get changed, but the change is instantaneous instead of smoothly animated. The reason for the extra logic in #1838 it's because the camera.padding property is not animatable, so we need to use a custom animation transition instead.

If I'm wrong and you see something different when you run your code, let me know!

cc @mfazekas

padding.mov

@dorthwein
Copy link
Contributor Author

dorthwein commented Apr 18, 2022

@naftalibeder - I think the missing piece is you're not setting an animation duration on the props or in setCamera in the given example. If you set the animationMode & an animationDuration it should gracefully add the padding.

To be clear, it does animate for me and is not the sudden jerking motion in your video. Try adding an animation duration and animation mode and let me know if that fixes it for you.

@naftalibeder
Copy link
Collaborator

@dorthwein So! I'm not sure what my issue was before, but you're right that the padding does seem to animate for map.camera.ease with timing easeInOut and linear. However, for map.camera.fly, padding does still jump.

PR-1844.mov

Whereas in the one I have in progress, it animates the padding smoothly:

PR-1838.mov

Am I missing something? Happy to retract my statement if I'm wrong. :) However if you don't mind the theft, I think I'll only use the lower-level animator for .flight.

@dorthwein
Copy link
Contributor Author

@naftalibeder - I have no issue with you using my changes in your PR and that being the one that ends up in main ;) - I have a hunch the flyTo jump is probably due to something else going on though.

I was able to reproduce the jump when using flyTo. My only concern with your implementation is the code seemed to diverge from main more in the UpdateItem.

I defer to @mfazekas on which he wants to pull in on this.

@naftalibeder naftalibeder changed the title Allowing ability to set Padding as stop in ios Allowing ability to set Padding as stop Apr 20, 2022
@naftalibeder naftalibeder added iOS iOS related tickets v10 labels Apr 20, 2022
@naftalibeder
Copy link
Collaborator

naftalibeder commented Apr 20, 2022

So I think we're going to keep the more complex logic for now to handle flyTo, and then if the bug gets fixed upstream in iOS, we can remove that to look more like this PR. Thanks!

Edit: See #1838 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS iOS related tickets v10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants