Skip to content
This repository has been archived by the owner on Sep 19, 2018. It is now read-only.

Update markerclusterer - moved to GitHub from SVN. #433

Merged
merged 2 commits into from
Jun 25, 2016

Conversation

oana-sipos
Copy link
Member

There was an issue with the markers on the Events map. I think it has to do with the fact that Google moved the repo from SVN to GitHub. I have updated the file + added the 5 markers (m1-m5). The images were added in folder static/img/* - I find it weird there was no other markers before, so if you spot a mistake, please flag it and don't accept the PR. Thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.114% when pulling 39abc0b on oana-sipos:master into c881805 on codeeu:master.

@mitio
Copy link
Member

mitio commented Jun 24, 2016

Hey @oana-sipos,

It's great that you spotted the issue and provided a fix!

You're probably right that the images stopped working since this URL is no longer available: http://google-maps-utility-library-v3.googlecode.com/svn/trunk/markerclusterer/images/m2.png I think we shouldn't have used that in the first place.

After a quick glance over the diff I think it looks fine and your solution to include the images in our repo is a lot better than to reference them from an external server.

I can't do much more, however, since I've stepped down from the webdev team at Code Week. I will give you write access to the repositories and you'll be able to merge this PR yourself should you decide so. I can also provide you with access to the production server so you or someone esle can deploy the fix.

MarkerClusterer.prototype.MARKER_CLUSTER_IMAGE_PATH_ =
'http://google-maps-utility-library-v3.googlecode.com/svn/trunk/markerclusterer/' +
'images/m';
MarkerClusterer.prototype.MARKER_CLUSTER_IMAGE_PATH_ = '../images/m';
Copy link
Member

Choose a reason for hiding this comment

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

Just one more thing on this change:

If markerclusterer.js is a third-party library and if you've updated the file with the latest version of the library, we shouldn't modify it in place. If the change on this line, regarding the path to the images set in MARKER_CLUSTER_IMAGE_PATH_ is made by you, you should revert it and leave it to the default and then set the proper value outside of this file, in another JS file.

Copy link
Member Author

Choose a reason for hiding this comment

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

You have a point. Will take a closer look at it tonight and revert it. Thanks!

@keikoro
Copy link

keikoro commented Jun 24, 2016

@mitio Just saw this and invited @oana-sipos to be a member of the org (hm, maybe you did so too – GitHub doesn't say if someone's already been invited).

Also just noticed that apparently, we're still using old organisation teams. I might look into restructuring those/removing old structures.

@mitio
Copy link
Member

mitio commented Jun 24, 2016

@kerstin Yeah, I also added her in the "Members" team (people with write and admin access).

If you're interested into restructuring the teams, please go ahead as you see fit. 🚢

@oana-sipos oana-sipos merged commit 5decb99 into codeeu:master Jun 25, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.114% when pulling c38a367 on oana-sipos:master into c881805 on codeeu:master.

@oana-sipos
Copy link
Member Author

@mitio Could you please deploy? Thanks 🍨

@mitio
Copy link
Member

mitio commented Jun 26, 2016

@oana-sipos I deployed it using the ./deploy-latest-coding-events script I wrote (as mentioned in the instructions).

Now no markers show on the map, however.

I will leave it deployed so you can track what might have caused it. I did a very quick investigation but I see no apparent problems aside from the fact that the markersclusterer.js is loaded from this URL in production: http://events.codeweek.eu/static/CACHE/js/4ed9097d9207.js which means the images would be searched for in http://events.codeweek.eu/static/CACHE/img/m... which is not correct. Maybe an absolute path needs to be used for them, e.g. /static/img/m. I think, however, that the problem is somewhere else since no marker images are attempted to be loaded and no markers appear to be drawn at all.

@mitio
Copy link
Member

mitio commented Jun 26, 2016

P.S. Here is the deploy log and the short deployment script.

@oana-sipos
Copy link
Member Author

Something is clearly weird in here. As far as I understand markerclusterer.js is loaded in production from http://events.codeweek.eu/static/CACHE/js/8121d734f1ac.js which doesn't fully make sense. I am trying to run everything locally and also deploy to heroku to see how it works in a prod env. An evening + night of traveling is awaiting for me, so it might take until tomorrow to fix it fully (am afraid will not have a connection anytime soon). If you think it's better, please revert the deploy to the previous commit. Thanks, keep in touch!
p.s. About deploying: guess you would need a private ssh key from me to be able to connect to root?

@mitio
Copy link
Member

mitio commented Jun 26, 2016

Things are different on the public site due to the fact that there's some concatenation and caching going on only on production. You might need to check out the actual configuration on the production server.

However, the file itself is being loaded, so that doesn't seem to be the problem. The issue is possibly somewhere else.

Let's leave this version up for now. No need to revert back to another broken version.

For deploying, I will need your public key, not the private one. The private key must stay only in your possession. You can check out this article or this one on the topic on generating the private-public key pair. When you've generated one, just email me the contents of your public key (most likely contained in ~/.ssh/id_rsa.pub).

@oana-sipos
Copy link
Member Author

I meant public of course :P E-mail sent to you with it. Just ping me when you add it. Thanks!

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.

None yet

4 participants