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

Replaced Mapbox with osmdroid (Nearby activity) #5403

Merged
merged 22 commits into from
Dec 24, 2023
Merged

Replaced Mapbox with osmdroid (Nearby activity) #5403

merged 22 commits into from
Dec 24, 2023

Conversation

kanahia1
Copy link
Contributor

@kanahia1 kanahia1 commented Dec 14, 2023

Fixes #5372

What changes did you make and why?
✔️ Replaced MapBox with OSMDroid
✔️ Tested most of the pre-existing features

Tests performed (required)

Tested betaDebug on Samsung S21 FE with API level 33.

Screenshots (for UI changes only)
https://github.com/commons-app/apps-android-commons/assets/114223204/501e79a8-bf8e-4a3a-8499-f0490e46af3c

🔧 Need to work on tests

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

It works great!
Hopefully unit tests can be fixed. :-)
I just added a few comments.
Thanks a lot for the nice code!

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

The location is now updated at least when restarting the app, thanks for fixing! 🙂

The unit tests are the next priority, I guess.

Minor thing: Would it be possible to close the pin labels at some point, for instance when tapping another pin, so that at most one label is open at any time? If difficult it can be done later, possibly by someone else, no worries.
Screenshot_20231217-115304.png

@nicolas-raoul
Copy link
Member

Feel free to add @Ignore to the tests that are failing, they can be solved later by anyone as another GitHub issue.
And pull from main to resolve the small conflict in app/build.gradle.
Then this pull request can be merged into the main branch. :-)

@kanahia1
Copy link
Contributor Author

Hi @nicolas-raoul , I have made the changes as advised by you and I have also fixed the marker click issue.

Marker.Click.mp4

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Thanks a lot @kanahia1 for this very needed change!
It works great.

@nicolas-raoul nicolas-raoul merged commit 5df18fb into commons-app:main Dec 24, 2023
@nicolas-raoul
Copy link
Member

I created #5408

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.

Replace Mapbox with osmdroid (Nearby activity)
2 participants