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

Update ShapeSource methods to make it usable with any cluster #1499

Merged
merged 4 commits into from
Sep 10, 2021
Merged

Update ShapeSource methods to make it usable with any cluster #1499

merged 4 commits into from
Sep 10, 2021

Conversation

tr3v3r
Copy link
Contributor

@tr3v3r tr3v3r commented Aug 22, 2021

Description

Since current implementation of getClusterExpansionZoom, getClusterLeaves and getClusterChildren uses querySourceFeatures / featuresMatchingPredicate to get desired data, it sets a limitation on functionality provided by native (original) corresponding methods. For now, obtaining needed data from this method is only possible for the clusters that are currently rendered on the map. But according to docs of the SDK it should be possible to use it on any cluster ( even deeply nested )

This PR updates these methods by passing the cluster itself not cluster_id.

Checklist

  • I have tested this on a device/simulator for each compatible OS
  • 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

Possible use case:
To zoom or just find a cluster for any single point on the map.

Solution:
Recursively use getClusterChildren(rootCluster) to find desired cluster and then with getClusterExpansionZoom(deeplyNestedCluster) smoothly zoom to it. ( For example, user searches some place on the map using search input )

Example

 <ShapeSource
     ref={shapeSourceRef}
     onPress={async event => {
        const { features } = event;
        const cluster = features[0];
     
        const featureCollection = await shapeSourceRef.current.getClusterChildren(cluster);

     }}
 
 
 

@tr3v3r
Copy link
Contributor Author

tr3v3r commented Aug 22, 2021

@ferdicus @mfazekas Hi!
Could you please take a look?

@mfazekas
Copy link
Contributor

@tr3v3r Thanks for the PR 👍 looks nice.

One issue could be backward compatibility. getClusterExpansionZoom (clused_id ) will break with the PR, trying to parse it. Not sure if existing interface does makes sense, but even if doesn't we need to add a warning about that. But I guess we can support both, either by kw argument is java, or just detecting the type of what's passed in.

What do you think?!

@tr3v3r
Copy link
Contributor Author

tr3v3r commented Aug 22, 2021

@mfazekas thanks for quick response!
Your proposal definitely make sense. I’ll add possibility to use methods with cluster id as well.
Thanks)

@tr3v3r
Copy link
Contributor Author

tr3v3r commented Aug 22, 2021

@mfazekas Also, I think we can add a comment/warning that cluster_id arg. is deprecated and will be removed in version 9+. What do you think?

@tr3v3r
Copy link
Contributor Author

tr3v3r commented Aug 23, 2021

@mfazekas PR updated.

Instead of adding some if / else statement inside native methods to detect the type of an argument I've just copied prev. methods implementation to reduce the number of possible bugs and make easier the process of removing this functionality in the future.

If we merge this I also provide PR that deletes deprecated functionality for release 9+.

Thanks for your time )

@tr3v3r
Copy link
Contributor Author

tr3v3r commented Aug 27, 2021

@mfazekas @ferdicus Hello guys! I hope you are well!

Did you have an opportunity to take a look at updates?

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.

@tr3v3r thanks this looks great to me, there are some notes in the review, otherwise it looks great to go!

Thanks for the nice work! 👍

CHANGELOG.md Show resolved Hide resolved
@ferdicus
Copy link
Member

thanks 🙇🏿

@ferdicus ferdicus merged commit 0cba0fc into rnmapbox:master Sep 10, 2021
mikalaiulasevich pushed a commit to OneSoil-Platform/maps that referenced this pull request Sep 23, 2021
…ox#1499)

* Update ShapeSource methods to make it usable with any cluster within ShapeSource source data

* Get back possibility to pass cluster_id as argument for backward compatibility

* Add more description into changelog, get rid of redundant check of zoom === -1, add check if mSorce exists on Android

* Fix description text
sdacunha pushed a commit to sdacunha/maps that referenced this pull request Oct 25, 2021
…ox#1499)

* Update ShapeSource methods to make it usable with any cluster within ShapeSource source data

* Get back possibility to pass cluster_id as argument for backward compatibility

* Add more description into changelog, get rid of redundant check of zoom === -1, add check if mSorce exists on Android

* Fix description text
@tr3v3r tr3v3r deleted the shapesource-methods-enhancement branch November 2, 2021 13:37
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