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 garden center icons #267

Merged
merged 4 commits into from May 11, 2016

Conversation

Projects
None yet
3 participants
@TheMapSmith
Contributor

TheMapSmith commented May 8, 2016

for #236

@samanpwbb

This comment has been minimized.

Member

samanpwbb commented May 9, 2016

@TheMapSmith hey Stephen, thanks for the contribution! It looks like one of your tests is failing. Can you add garden-center to the layout json file here: https://github.com/mapbox/maki/blob/master/layouts/all.json

@TheMapSmith

This comment has been minimized.

Contributor

TheMapSmith commented May 9, 2016

@samanpwbb Ah, thanks for the heads up. Should be good now

@natslaughter

This comment has been minimized.

Contributor

natslaughter commented May 9, 2016

@TheMapSmith Thanks so much for the Pull Request! Here is some initial feedback:

I think the watering can works as a concept.

Overall Shape

The 15px has a handle, whereas the 11px doesn't. It's important that the two icon files have the same, overall shape. If you think the icon works with out the handle, then it should be removed from the 15px icon. Or, if you think it needs the handle, the 11px icon needs a handle. I personally think the handle really adds to the overall shape.

I wonder if the spout and can connection could just be a diagonal line, instead of the spout beginning straight, and then turning upwards. Also, the end of the spout could have a more nuanced shape that could help with the overall silhouette.

screen shot 2016-05-09 at 12 47 58 pm

15 pixel icon file

Overall, I think it's a good start. A few things I would try would be to create a little more space between the can and the handle. At 100% scale, it's a little hard to read, but almost there ... and maybe after you solve some of the pixel alignment issues that might solve this.

The icon needs to be more pixel aligned to avoid blurriness.

screen shot 2016-05-09 at 12 22 33 pm

Some of the 'line weights' are not consistent, and we almost always try to keep these at 1pixel.

screen shot 2016-05-09 at 12 23 02 pm

Some of the curves and arcs are close, but they need to maintain these circle radiuses.

screen shot 2016-05-09 at 12 23 41 pm

11 pixel icon file

As mentioned above, it would be great if a handle could fit in here.

The pixel alignment is much better in this file.

I would try and use more space that is available if possible. It could help with creating more space in-between the can and the handle, more space between the can and the spout, and creating a handle.

screen shot 2016-05-09 at 12 58 28 pm

The curves and arcs are close.

screen shot 2016-05-09 at 1 34 49 pm

Let me know if you have any questions at all.

Thanks again for the submittal and we look forward to hearing back!

@TheMapSmith

This comment has been minimized.

Contributor

TheMapSmith commented May 9, 2016

Awesome feedback, thanks @natslaughter! I'll take a look at your suggestions and send an update.

@TheMapSmith

This comment has been minimized.

Contributor

TheMapSmith commented May 9, 2016

@natslaughter I think I managed to incorporate all your suggestions. Please let me know what you think 👍

@natslaughter

This comment has been minimized.

Contributor

natslaughter commented May 10, 2016

@TheMapSmith
Hi Stephen,
Thanks for the update. It looks great!

A few final things:

15 pixel icon file

I noticed there are a few curves and arcs slightly off. Also, at 100%, the space between the top handle and the can is a bit hard to read. I might suggest bringing the top handle up to meet the pixel grid, and you can also create a little bit of breathing room too.

screen shot 2016-05-10 at 2 51 34 pm

11 pixel icon file

The only issue I can see is at 100%, it loses it's silhouette and clarity. You might need to take your handle and spout stem widths down to .5 pixels. By doing this, you get a little more space so that each watering can element reads (the watering can on the right has .5 widths).

screen shot 2016-05-10 at 2 59 30 pm

Stephen, thanks again for the contribution. I think you're very close!

@TheMapSmith

This comment has been minimized.

Contributor

TheMapSmith commented May 11, 2016

@natslaughter I've implemented your latest suggestions. I want to thank and commend you for such a welcoming, thorough, and helpful code review. I felt encouraged to make my design better, felt grateful that a sharp-eyed designer was guiding my learning, and never felt bad for not getting it just right. I hope these last changes meet the guidelines (but I won't feel bad if some of my curves still need some nudging or aligning). I'm looking forward to contributing more in the future.

@natslaughter natslaughter merged commit f7296bd into mapbox:master May 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@natslaughter

This comment has been minimized.

Contributor

natslaughter commented May 11, 2016

@TheMapSmith
Stephen, thanks so much for your great work! Just merged your garden-center icons!
It was a pleasure working with you. I would love to see you design some more icons as your schedule permits. Let me know if you have any thoughts or questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment