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

Add a google map and index to view #82

Closed
wants to merge 1 commit into from

Conversation

ETNOL
Copy link
Contributor

@ETNOL ETNOL commented Aug 19, 2015

issue #62
This is the initial conversion of my google map script to consume the pizzeria geojson and spit out a map full of markers.

Its js code from my first days of dev so it could a major refactor which I'd be happy to perform soon. I also brought in psQuery to handle the ajax request because I'm ajax-lazy. I think it can be branched to a 'gh-pages' branch in the current state and be available through Github pages at http://stevekinney/github.io/pizza/

image

@alanbsmith
Copy link
Collaborator

This is cool! Thanks for the PR, @ETNOL. We're using pizza_map.geojson for our map. Our goal for PRs is to have a fairly small core of central functionality and also to display example client applications that consume this dataset.

I think this PR really fits well into that second category. So we had two ideas, one would be to build this out into a separate client application that uses this data, or we could create an 'examples' folder where this could live and serve as an example for people wanting to build something with this dataset.

Let us know what you think and we can go from there. And thanks again!



window.onload = function () {
psQuery.ajax({url: 'https://raw.githubusercontent.com/ETNOL/pizza/master/pizza_map.geojson', success: jsonSuccess})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks great. The only thing I would change is this line. I would set the url to './pizza_map.geojson' so anyone could grab from their dataset when they pull it down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either option sounds good! My reasoning for using github to host the json
data was to make it easier to keep updated. I'd probably want to use the
original source repo instead of my fork though so I wouldn't need to pull
down updates. My original map on my personal site wasn't geared towards
anything specific so I might just replace it with this instead. If I do,
should I make a pull request with its url in an examples.md or just drop
the code in a folder like "examples/pizza-map/{codebase}"?

On Fri, Aug 21, 2015 at 3:31 PM, Alan B Smith [email protected]
wrote:

In js/map.js
#82 (comment):

  •    props.pizzeria +
    
  •    '<br />' +
    
  •    props.address +
    
  •    '<br />' +
    
  •    props.city +
    
  •    '<br />' +
    
  •    '<a href=' + props.website + '>' + props.website + '</a>'
    
  •  '</div>';
    
  • };
  • return setMarker;
  • };

This looks great. The only thing I would change is this line. I would set
the url to './pizza_map.geojson' so anyone could grab from their dataset
when they pull it down.


Reply to this email directly or view it on GitHub
https://github.com/stevekinney/pizza/pull/82/files#r37680332.

@cluhring
Copy link
Contributor

cluhring commented Sep 8, 2015

Hey @ETNOL - I just added a d3_map directory into "examples/maps/" - Please feel free to throw your google_map directory in there with mine.

@stevekinney
Copy link
Owner

Hey y'all—what's the status with this PR? Is it ready to go?

@stevekinney
Copy link
Owner

Just checking in to see if this is ready to merge. 👼

@ETNOL
Copy link
Contributor Author

ETNOL commented Jun 8, 2016

There is a D3 map in the codebase so lets leave this out.

@ETNOL ETNOL closed this Jun 8, 2016
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.

4 participants