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

move embassy icon to SVG #1235

Merged
merged 1 commit into from
Jan 19, 2015
Merged

Conversation

nebulon42
Copy link
Contributor

Implements #1165 by changing the icon for amenity=embassy to SVG.
Icon: nebulon42/osmic/embassy-16.svg

before
embassy_before

after
embassy_after

@nebulon42 nebulon42 mentioned this pull request Jan 16, 2015
66 tasks
@matkoniecz
Copy link
Contributor

Please, test it with tall name like "ÉÉÉÉÉÉ ÉÉÉÉÉÉ ÉÉÉÉÉÉ" (text-dy is too low).

@nebulon42
Copy link
Contributor Author

The text would have worked, but I nevertheless increased text-dy by 1.

@matkoniecz
Copy link
Contributor

Weird, according to my test it was failing.

@nebulon42
Copy link
Contributor Author

With your tool? I changed the text to ÈÈÈÈÈ, but I don't know, which value do you suggest?

z19
embassy_text

@pnorman
Copy link
Collaborator

pnorman commented Jan 18, 2015

I'd give it one more pixel over what you have in the image - because the text isn't uniformly tall, it looks to not be hitting the tallest point.

@nebulon42
Copy link
Contributor Author

Right, I did that already, but didn't update the preview.

@nebulon42
Copy link
Contributor Author

I have updated the icon to a lighter version that looks better and is more similar to the old icon.

@matthijsmelissen
Copy link
Collaborator

The latest preview does not match with the icon in the repository. Which one is the correct version?

@nebulon42
Copy link
Contributor Author

Which one is the correct version?

The one in the inital post should match with the repository and the file in this PR.

@matthijsmelissen
Copy link
Collaborator

The preview in #1235 (comment) is different from the file in the repository though. The waves are the other way around. Can I ignore this preview?

@nebulon42
Copy link
Contributor Author

Yes, this was the first version, which has been superseded. The comment was intended to show to @mkoniecz that I tested with this particular name.

I probably should not update the first comment anymore to avoid confusion about updates.

@matthijsmelissen
Copy link
Collaborator

The comment was intended to show to @mkoniecz that I tested with this particular name.

I actually like the updated first post. In any case I merged this now.

matthijsmelissen added a commit that referenced this pull request Jan 19, 2015
@matthijsmelissen matthijsmelissen merged commit 81a9f70 into gravitystorm:master Jan 19, 2015
@nebulon42 nebulon42 deleted the svg-embassy branch January 19, 2015 20:12
@matkoniecz
Copy link
Contributor

I actually like the updated first post.

I also prefer first post to contain current state (or at least - link to post with a current state), not what was initially submitted.

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