-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixes #545 - Add support for campaigns #4423
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4423 +/- ##
============================================
- Coverage 10.34% 10.29% -0.06%
Complexity 479 479
============================================
Files 342 342
Lines 13201 13269 +68
Branches 1083 1089 +6
============================================
Hits 1366 1366
- Misses 11766 11834 +68
Partials 69 69
Continue to review full report at Codecov.
|
9b19464
to
b91355e
Compare
d87b663
to
bbe2b6b
Compare
- Show monuments in maps along with nearby
Hi @ashishkumar468 , I have been trying to test this PR, but have encountered a few roadblocks. I am running a Samsung Galaxy S20 FE on Android 11.
On the other hand, I just tried v3.0.2 Nearby and that works fine for me on the same device. |
@misaochan I will look into it, for now can you try a different device. Also, you would need to enable the wlm from the settings. |
Tried with emulator, Pixel 3a running Android 10, encountered the same issue. :/ |
Thanks for the PR @ashishkumar468 :) I have tested it. At first the campaign setting was off but WLM setting was on. With these settings no campaign banner appeared and nearby loads forever (even pins didn't appear). |
Thanks for testing @neslihanturan ! @ashishkumar468 I was unaware that there were 2 separate settings. Aside from the issues Neslihan mentioned, I also think we should consolidate both into one setting - campaigns should automatically include WLM. If a separate flag is needed for testing WLM, I think we should just hardcode it to "on" for testing purposes for the time being. Also, I think we all agreed that the WLM banner in Contributions should just utilize the existing design (for campaign cards), right? :) It is just the functionality that we are changing - if the user taps on the banner they should be brought to Nearby. |
Tested again today with Neslihan's method (both campaign and WLM setting turned to on), it works for me! Will consolidate my feedback below with a checklist:
The summary says As discussed, I would recommend using a sandbox "monument" for testing template additions, as suggested by @nicolas-raoul at #545 (comment) . @nicolas-raoul , do you know how we can add P1435 and also a location, to the sandbox Wikidata item that you mentioned? I'm not sure how to add Wikidata properties. |
I added |
Thanks a lot @nicolas-raoul ! For some reason I didn't think to scroll all the way down... :) |
Hey @misaochan , I have tested it under Android 11 Nexus 6 device. Search this area button worked well and WLM markers appeared: |
… monuments - [P1435, P2186, P1459, P1460, P1216, P709, P718, P5694] 2. Append WikiData QID to descriptions template
app/src/main/res/values/strings.xml
Outdated
<string name="display_monuments">Display monuments</string> | ||
<string name="wlm_month_message">It\'s Wiki Loves Monuments month!</string> | ||
<string name="learn_more">LEARN MORE</string> | ||
<string name="wlm_campaign_title">Its WML season</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just say "Wiki Loves Monuments"
Tried testing again today on my real device, a few notes:
Once we are done with testing (after this PR is merged), we need to:
|
Hey @misaochan , it worked for me with search this area for Ireland and Australia with pyhsical Android10 device. Here are screenshoots: |
Hi @misaochan , the usual Nearby items, do show the pre-filled desc as they used to before. For the WLM pins, as I have mentioned before, the P6375 used for the "streetAddress" does not return anything for my location, attaching a sample response, and therefore that field in the details or in the Upload step is omitted. Sharing a sample response for WLM query - https://gist.github.com/ashishkumar468/5d2eac4844d3c5c600de467e2558c6cb As you can see the streetAddress property is empty. |
…, continue showing the nearby items if that succeeds
Notes from discussions (on Zulip and mailing lists) and tests:
|
Made separate issues for everything else. Only thing pending for merge is "We have decided that due to the frequent lack of P6375, we will just use the standard label+desc used in non-WLM points for WLM points as well". |
…ead of relying on P6375
Great job, @ashishkumar468 ! Merging :) |
Description (required)
Query Wikidata to fetch monuments and show them in Nearby
Fixes #545
What changes did you make and why?
Tests performed (required)
Tested the build prodDebug on the local emulator.- Able to see monuments for location (50.9709677364145, 22.776031494140625)
Feature
Screenshots (for UI changes only)