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

Expose GoogleMaps iOS SDK icon property to improve Marker rendering performance #2650

Merged
merged 4 commits into from
Jan 8, 2019

Conversation

victormartingarcia
Copy link
Contributor

@victormartingarcia victormartingarcia commented Jan 7, 2019

Does any other open PR do the same thing?

This PR is based on #1491 changeset

It hasn't been merged (presumably unsolved conflicts), but I see Marker tracksViewChange property is already exposed on master branch, so it makes sense to expose Marker icon property as well

What issue is this PR fixing?

Rendering Markers on iOS using Google Maps have some performance issues (see #2616, #1031, #2575 ..)
Some people have solved that with some extra logic tweaking tracksViewChange property (see #1031 (comment) ), but as Google explains in the SDK docs, for static Markers is much better to use icon property.

Using this icon property solves all iOS performance issues, making the map much smoother :)

How did you test this PR?

This fix has been tested on iPhone simulator and real devices (iPhone 6 and iPhone X) with extremely good results.

On Android, icon property works like image property, as Android SDK does not differentiate between static image Markers and View-based ones

Example

<Marker
    coordinate={{
        latitude: this.state.myLatitude,
        longitude: this.state.myLongitude
    }}
    icon={require('../Images/myIcon.png')}
/>

@rborn
Copy link
Collaborator

rborn commented Jan 7, 2019

Thanks for the PR ❤️

I wonder of we could unify the icon/image props between the platforms? so we use one name only? Or on iOS we can use both image and icon ?

@alvelig thoughts ? and 🐽
LGTM

@victormartingarcia
Copy link
Contributor Author

@rborn on iOS you can use both image and icon for different purposes (icon only works for static images)

Let me check if I can make it compatible with android, for not having to differentiate it on Marker props

@victormartingarcia
Copy link
Contributor Author

I managed to support icon property on android as well, so no need to differentiate platform on Marker definition ;)

Using icon on android is exactly the same as using image, as android Google Maps SDK does not differentiate between static image Markers and View-based ones

@rborn
Copy link
Collaborator

rborn commented Jan 7, 2019

@victormartingarcia looks good, thanks.
One question though: is there anything we need to do to handle @2x/@3x images? How do the marker decide the size of the icon? (maybe we should add some docs saying what's the ideal marker icon size if google specifies something about it?)

@victormartingarcia
Copy link
Contributor Author

victormartingarcia commented Jan 7, 2019

One question though: is there anything we need to do to handle @2x/@3x images?

I've tested and @2x/@3x images are handled automatically 👍

How do the marker decide the size of the icon?

Just like with image, using this icon property shows a Marker with the same size as the source picture.

(maybe we should add some docs saying what's the ideal marker icon size if google specifies something about it?)

Haven't found anything about Marker size in Google Maps SDKs docs...

@rborn
Copy link
Collaborator

rborn commented Jan 7, 2019

@victormartingarcia let's wait a little for @alvelig to reply (if) and then we'll merge 🎉

@christopherdro
Copy link
Collaborator

Related to #1491

@christopherdro christopherdro merged commit 70eeb2d into react-native-maps:master Jan 8, 2019
@christopherdro
Copy link
Collaborator

I'm not sure why this wasn't caught in the tests that ran on this PR but I am following up with the following #2651

@lone-cloud lone-cloud mentioned this pull request Jan 14, 2019
sttz added a commit to sttz/react-native-maps that referenced this pull request Feb 24, 2019
…x.d.ts

### Does any other open PR do the same thing?

No.

### What issue is this PR fixing?

With TypeScript, trying to use the Marker's icon property results in an error, since the `index.d.ts` does not yet declare the property that was added in react-native-maps#2650.

### How did you test this PR?

Tested by editing the `index.d.ts` locally. Since there are no functional changes, this is a trivial change.
christopherdro pushed a commit that referenced this pull request Mar 6, 2019
### Does any other open PR do the same thing?

No.

### What issue is this PR fixing?

With TypeScript, trying to use the Marker's icon property results in an error, since the `index.d.ts` does not yet declare the property that was added in #2650.

### How did you test this PR?

Tested by editing the `index.d.ts` locally. Since there are no functional changes, this is a trivial change.
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.

None yet

3 participants