Skip to content

Conversation

uberbuilder
Copy link

In response to Issue:1552.

I'm not sure if this file needs to be changed in a test somewhere or some other source svg or something... I just thought I would try and make it easy for you @mourner

Renamed dist/images/marker-icon@2x.png to dist/images/marker-icon-at-2x.png.

Renamed `dist/images/marker-icon@2x.png` to `dist/images/marker-icon-at-2x.png`.
@lookfirst
Copy link
Contributor

It is referenced in the JS code.

@uberbuilder
Copy link
Author

sigh...

@lookfirst
Copy link
Contributor

I'd just leave this pull request open and add a commit with the js file change. Reference this PR in the commit and it'll automatically get added to it.

I looked in all files in the project referencing: `marker-icon@2x.png` - and I found that this string was only represented in the marker.svg - no source .js files.

I have since edited the marker.svg with the new filename (`marker-icon-at-2x.png`)and I'm re-submitting the pull request.
@uberbuilder uberbuilder reopened this Mar 27, 2013
@uberbuilder
Copy link
Author

I searched the entire project for "marker-icon-at-2x.png" or even just "marker-icon-at-2x".

Also ran a git log -Smarker-icon@2x.png - and I never found any files other than marker.svg that references this string.

@uberbuilder
Copy link
Author

The only other place I can seam to find any reference to marker-icon@2x.png is in the gh-pages branch under the /src directory.

@lookfirst
Copy link
Contributor

https://github.com/Leaflet/Leaflet/blob/master/src/layer/marker/Icon.Default.js#L23

On Wed, Mar 27, 2013 at 3:17 PM, Jeremy Iglehart
notifications@github.comwrote:

I searched the entire project for "marker-icon-at-2x.png" or even just
"marker-icon-at-2x".

Also ran a git log -Smarker-icon@2x.png - and I never found any files
other than marker.svg that references this string.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1553#issuecomment-15556741
.

@uberbuilder
Copy link
Author

@lookfirst Thanks, I found it in three places.

/src/layer/marker/Icon.Default.js
/dist/leaflet-src.js
/dist/leaflet.js

I can change it in all three... but it looks like I should just change the /src - and then the project should be "compiled" - I've not done that yet on this library, I also don't have experience with the test suite. So, I'll change the /src reference and then pass this on to @mourner

@lookfirst
Copy link
Contributor

Yes, you just need to change the src directory. The dist directory (imho) shouldn't be checked in anywhere. You generate the code in the dist folder by doing this in the leaflet top level directory:

npm install
./node_modules/.bin/jake

Thanks to @lookfirst I installed the test suite and ran jake - everything passed just fine.  Updated the two files in the dist and now I'm committing them to the pull request.  If you want these changes in the mean-time pre-merge (assuming that the file name change is accepted), feel free to use: https://github.com/uberbuilder/Leaflet/tree/marker-icon-rename
@uberbuilder
Copy link
Author

@lookfirst Thanks for the mini-tutorial... lol, I'm sure you could have done this yourself - but I enjoyed the process and learning experience.

@lookfirst
Copy link
Contributor

Glad I could help and thanks for taking the reigns on submitting the pull request. =)

@uberbuilder
Copy link
Author

Uhg - diffstats for src are WAY off - line ending problem again.


Edit:

This is not a problem I was wrong - this is referencing the dist/leaflet-src.js - which makes sense. Maybe I really shouldn't be committing this file... It seams like project bloat to me. What do you think @mourner - how would you like me to do this in the future?

@mourner mourner closed this in ad4f0e6 Apr 19, 2013
@mourner
Copy link
Member

mourner commented Apr 19, 2013

Did the rename myself as the pull included the built files (which is unwanted) and marker-icon-2x is a better name than marker-icon-at-2x. Still thanks for the report and pull!

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