-
Notifications
You must be signed in to change notification settings - Fork 32
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
Auto-complete address and coordinates of locations #1744
Conversation
Code Climate has analyzed commit 9c219c2 and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 45.4% (50% is the threshold). This pull request will bring the total coverage in the repository to 74.4% (0.0% change). View more on Code Climate. |
c2aee7e
to
24267a2
Compare
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.
Very nice work!
Tested and seems to work correctly 👌🏼
I think you can adjust the help text of the longitude/latitude field in the poi_form, as they are now inferred live :) Maybe something like Geographische Länge und Breite werden automatisch aus der Adresse abgeleitet
.
Further I'm a little bit worried about people who want to set longitude and latitude to a different value as the address. In this case changing the Address would mean to overwrite the longitude and latitude value. Maybe the users should be able to opt-out from inferring by adding a tick box Länge und Breite aus der Addresse ableiten
?
This would also make the help text for longitude and latitude irrelevant, I guess.
@sarahsporck Now the tick box is added so users can choose whether the coordinates are to be auto-filled. |
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.
Thank you! This pr works as expected, I just found a few typos and smaller problems:
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.
Thanks a lot, really cool feature! 🚀
Just because you linked issue #1000 as fixed: I'm very sorry if the issue was misleading - the checked items were not the tasks to work on, but the tasks that were already done... so in theory I already considered the previous solution with fetching the coordinates after form submission sufficient - nevertheless very cool feature to fetch them dynamically! 👍
I just wanted to point out that this PR does not fix the issue - it just improves one part of it which was already done.
3fb85ba
to
2fef44e
Compare
73e4f70
to
07c2ebf
Compare
@timoludwig Sorry that I have misunderstoot the issue. I'll now add other (not yet checked) features 🙂 |
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.
Thanks! A few small details, then this PR is ready to be merged! 🎉
I'll now add other (not yet checked) features 🙂
Well, I mean those are just ideas, if you notice that some of them are too much work for the benefit they provide, you can also edit the issue and explain why you reject a specific point 😁
Now the ajax call for the coordination will be called if street and postcode are filled (city is not anymore an necessary firing condition). When coordinates are found from the street and postcode, this proves (I think) the validity of street input. Users can know something is not correctly given, seeing an error from this calculation. From those coordinates the city will be calculated. The calculated city will be auto-filled (or replace the old input if the new address is in another city). The country field will be now auto-filled too. |
Oh oh... I read this comment just now...... 😅 |
c10a5a9
to
e0e90a8
Compare
e0e90a8
to
cd973c0
Compare
Co-authored-by: David Venhoff <venhoff@integreat-app.de> Co-authored-by: Timo Ludwig <ludwig@integreat-app.de
cd973c0
to
9c219c2
Compare
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.
Thanks a lot, I really think this makes creating POIs much easier! 🚀
I had a few ideas for improvement which were outside the area GitHub allowed me to post suggestions for, so I directly pushed to your branch 😇
I tried to explain my changes in the comments below:
@timoludwig |
Short description
Currently coordinates of a POI appears (if left blank) first after the POI is saved.
This PR auto fills coordinates live while editing when the address of POI is completed.
Proposed changes
Side effects
Resolved issues
Part of: #1000
Pull Request Review Guidelines