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 special icon for shop=trade, shop=wholesale #3361

Merged
merged 1 commit into from
Aug 30, 2018
Merged

Add special icon for shop=trade, shop=wholesale #3361

merged 1 commit into from
Aug 30, 2018

Conversation

Adamant36
Copy link
Contributor

@Adamant36 Adamant36 commented Aug 23, 2018

@Tomasz-W Tomasz-W mentioned this pull request Aug 23, 2018
26 tasks
@Tomasz-W
Copy link

Tomasz-W commented Aug 23, 2018

@Adamant36 Can you try with asymmetrical, but pixel-aligned version?:

shop trade2 shop trade2

Gist link: https://gist.github.com/Tomasz-W/79e6deb43381777ed666e8911c240743

@meased
Copy link
Contributor

meased commented Aug 23, 2018

Definitely needs pixel alignment.

@lakedistrictOSM
Copy link

There's also the pixel aligned icon that I designed, linked in the original issue. However Tomasz's asymmetric version is an improvement on his original.

I'd also like to see this icon used for cash and carry shops (shop=wholesale).

@Adamant36
Copy link
Contributor Author

Asymmetrical version 1
shop asymmetrical version 1
Asymmetrical version 2
asymmetrical version 2
I think I prefer the first version

I'm more then willing to add it as the icon for shop=wholesale also. It seems like the tag has enough uses. As long as others agree.

@kocio-pl
Copy link
Collaborator

Could you also test symmetric pixel aligned version?

shop=wholesale makes sense for me too - not too many uses, but it's fast growing:
taghistory 29

@Adamant36
Copy link
Contributor Author

@kocio-pl, I thought thats what I tested in the message above yours? Is that not what @Tomasz-W provided a few messages up or are talking about @lakedistrictOSM's icon in the original issue. I know nothing about how icons and pixel alignment work.

I'll add shop=wholesale then once the icon gets worked out.

@kocio-pl
Copy link
Collaborator

Well, I thought @lakedistrictOSM wrote it because his icon has not been tested. Which one did you use in the original rendering test?

@Adamant36
Copy link
Contributor Author

@kocio-pl the original rendering test was done with @Tomasz-W's original version. Here's a test of @lakedistrictOSM's versions.
trade lakedistrict version

@Adamant36
Copy link
Contributor Author

Adamant36 commented Aug 25, 2018

So just to recap

Tomasz-W symmetrical
tomasz-w original
Tomasz-W asymmetrical version 1
shop asymmetrical version 1
Tomasz-W asymmetrical version 2
asymmetrical version 2
lakedistrictOSM pixel aligned symmetrical
trade lakedistrict version

@kocio-pl
Copy link
Collaborator

I like @Tomasz-W versions because of these nice details on packages. Probably asymmetrical v2 is nice, because it shows more diversity between packages, but all the icons here are good enough. My preference list goes as follows:

  1. asymmetrical v2
  2. asymmetrical v1
  3. symmetrical v1 (@Tomasz-W )
  4. symmetrical v2 (@lakedistrictOSM )

but I have harder problems to solve and leave it just as a hint. Whatever you will decide this this icon is OK for me.

@Adamant36
Copy link
Contributor Author

Adamant36 commented Aug 25, 2018

@kocio-pl OK. Thanks.

@Tomasz-W and @lakedistrictOSM What do you both think?

@Tomasz-W
Copy link

@Adamant36 It's always harder to rate effect of your own work, so I believe in @kocio-pl "ranking" ;) . I've made v2 version, because v1 to be pixel-aligned has to have a tape shape in weird position, so I think my v2 without it might be even better.

@polarbearing
Copy link
Contributor

asymmetrical v2 works best for me too, and I agree to use the icon for wholesale as well.

@lakedistrictOSM
Copy link

They're all readable for me. The labels on Tomasz-W's designs are a nice touch.

@Adamant36
Copy link
Contributor Author

@kocio-pl If I add something new that is semi major to a PR like I am doing here by adding shop=wholesale is it usually the policy to add another commit for it and update the commit message to reflect it? Or do I just push the change, leave all the commit stuff the same, with a single commit, and mention it in the originally message?

@kocio-pl
Copy link
Collaborator

There's no policy for that. I guess if you enhance the title it should be enough, I just will wait a bit longer to not take anyone willing to comment by surprise.

@Adamant36 Adamant36 changed the title Add special icon for shop=trade Add special icon for shop=trade and shop=wholesale Aug 27, 2018
@Adamant36 Adamant36 changed the title Add special icon for shop=trade and shop=wholesale Add special icon for shop=trade, shop=wholesale Aug 27, 2018
@Adamant36
Copy link
Contributor Author

Adamant36 commented Aug 27, 2018

@kocio-pl It should be good to go now. I left the icon name as trade for both of them because I couldn't come up with anything better. Also, I didn't upload test photos of the new trade icon for shop=trade, but I did test it on my end just in case and it works fine.

@kocio-pl kocio-pl merged commit 0ba5691 into gravitystorm:master Aug 30, 2018
@kocio-pl
Copy link
Collaborator

Thanks! My independent tests are also OK.

@Adamant36 Adamant36 deleted the trade branch August 31, 2018 08:27
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.

Add icon for shop=trade (and potentially shop=wholesale)
6 participants