Skip to content
This repository has been archived by the owner on Nov 9, 2023. It is now read-only.

Add changeMarker function to library #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maxscheiber
Copy link

A couple of comments:

  1. This is the first pull request I've ever made (as well as the first open source contribution I've ever made). Hopefully the code style is acceptable and whatnot. Feel free to hit me up with any advice / constructive criticism.
  2. Disregard commit e48b967.
  3. Motivation for the fork and pull request: I've been using your truly awesome library for a project for my internship. I've needed to dynamically update markers while maintaining their order, so invoking removeMarker and addMarker just wouldn't do it (it changed the order of markers). To solve that, I wrote up a changeMarker method.
  4. There'd be lots of shared code between createMarker and changeMarker, so I refactored it into an initializeMarker helper function.

@hpneo
Copy link
Owner

hpneo commented Aug 19, 2013

Hi, I'm checking your code and I think it might fail in some cases. I will post some comments in your code, but first you need to apply your changes in lib/gmaps.markers.js since gmaps.js is a generated file.

@maxscheiber
Copy link
Author

Alright, thanks for letting me know. I feel kind of silly for one, not reading the contribution guidelines, and two, not running your test suite. I'll take a look at your comments later tonight and possibly issue a fresh pull request with added examples.

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

Successfully merging this pull request may close these issues.

2 participants