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

need person's location add using google maps #48

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

dulajsan
Copy link

Add GPS location when someone submit their needs. It is optional.

@dulajsan dulajsan closed this May 28, 2017
@dulajsan dulajsan reopened this May 28, 2017
-- Table structure for table `donations`
--

CREATE TABLE `donations` (
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add a migration use artisan cli tool , otherwise its going to be diffcult to manage the DB upgrades / changes

Copy link
Contributor

@gayanhewa gayanhewa left a comment

Choose a reason for hiding this comment

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

Once the migrations are added we should be good to go with this PR

app/Need.php Outdated
@@ -12,6 +12,6 @@ class Need extends Model
* @var array
*/
protected $fillable = [
'name', 'telephone', 'address', 'city', 'needs', 'heads'
'name', 'telephone', 'address','latlon', 'city', 'needs', 'heads'
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO latlon is a bit unreadable at first sight. Maybe something like geolocation?

Copy link
Author

Choose a reason for hiding this comment

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

okay i will change

</div>



Copy link
Contributor

Choose a reason for hiding this comment

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

Too many blank lines.

<button type="button" onclick="getLocation()" class="btn btn-primary">සිතියමට ඇතුල් කරන්න</button>
<p id="demo">&nbsp;</p>
<div id="mapholder" style="width:55%;height:300px">
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the Maps API to get the general area from the address input above?

https://developers.google.com/maps/documentation/geocoding/intro

Copy link
Author

Choose a reason for hiding this comment

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

yes we can. but if some one enter a wrong address, it will be a problem

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It could be more work at the moment, but can something like the following be done?

  1. User enters an address
  2. An attempt is made to obtain the geolocation using that address.
  3. If the locations are received, the map is refreshed to show that location.
  4. If the location is wrong on the map, the user adjusts it.

Maybe this can be an improvement after these current changes are merged.

Copy link
Author

Choose a reason for hiding this comment

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

That will be fine. i will work on it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

there is an issue to add MAP API

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Looks like there are several issues with different scopes on Maps. @danishka does this cover what you had in mind for #50 ?

Also #6 also seems to be similar in scope to this.

Copy link
Contributor

@chamilad chamilad left a comment

Choose a reason for hiding this comment

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

Please investigate the possibility of getting the geocode from the address itself.

@dulajsan
Copy link
Author

updated to getting geocode from address. please review it and notify me

function showError(error) {
switch(error) {
case "ZERO_RESULTS":
x.innerHTML = "ඔබගේ ලිපිනය සොයාගත නොහැක . රතු පැහැති සලකුණ ඇදගෙන යාමෙන් ඔබ සිටින ස්ථානය ලකුණු කල හැක "
Copy link
Contributor

Choose a reason for hiding this comment

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

Would these have to be i18nized? Not sure if that PR was merged.

Copy link
Author

Choose a reason for hiding this comment

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

these are the defined errors of the geocode. i checked those errors and printed customized errors messages for these defined errors

Copy link
Contributor

@ishanthilina ishanthilina May 30, 2017

Choose a reason for hiding this comment

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

@chamilad Do we have i18n support at the moment? Does resources/lang contain them?

Edit : Seems https://github.com/reliefsupports/reliefsupports.org/pull/122 gives i18n support.

@chamilad
Copy link
Contributor

@dulajsan Please include a screenshot of the UI if possible. Thanks for implementing this! I think this will also make a good starting point for a broader map feature.

@dulajsan
Copy link
Author

image

@danishka
Copy link
Collaborator

@dulajsan appreciate if you can fix conflict and test once with latest code in your local.

@heimdallrj heimdallrj added the DONE label Jun 1, 2017
@danishka
Copy link
Collaborator

danishka commented Jun 5, 2017

@thinkholic is this already in production?
I did not noticed it yet.

}
}

</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

@dulajsan please get an update from dev and add the translations for these

@@ -13,7 +14,8 @@ class AppServiceProvider extends ServiceProvider
*/
public function boot()
{
//
Schema::defaultStringLength(191);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, this is due to the multi-byte utf support that's needed. But we need to find a mechanism to go around the default string length problem and move this concert someplace that makes sense.

naveenmadhawa added a commit that referenced this pull request Jun 6, 2017
Geolocation integration forked from PR #48
@@ -137,19 +138,19 @@ function showPosition(position) {
function showError(error) {
switch(error) {
case "ZERO_RESULTS":
x.innerHTML = "ඔබගේ ලිපිනය සොයාගත නොහැක . රතු පැහැති සලකුණ ඇදගෙන යාමෙන් ඔබ සිටින ස්ථානය ලකුණු කල හැක "
Copy link
Author

Choose a reason for hiding this comment

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

added i18n support. Now we can expose requester location.

@dulajsan
Copy link
Author

dulajsan commented Jun 8, 2017

image

@danishka
Copy link
Collaborator

@dulajsan I have tested in staging and its not convenient as we search in Google maps.
Is it possible to type and search for a location?

@dulajsan
Copy link
Author

@danishka yes we can type in address field. Then the map will focused to relevant address. ex: If we type nugegoda, the map will focused to nugegoda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants