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

racetrack #549

Merged
merged 2 commits into from
Jun 16, 2021
Merged

racetrack #549

merged 2 commits into from
Jun 16, 2021

Conversation

tristen
Copy link
Member

@tristen tristen commented Jun 4, 2021

🤔 Not sure how I feel about this one as its pretty small.

Link to demonstration

https://api.mapbox.com/styles/v1/tristen/ckpins0oc0hpo18qwnjtodc6h.html?fresh=true&title=view&access_token=pk.eyJ1IjoidHJpc3RlbiIsImEiOiJjajZoOXU4Z2kwNnppMnlxd2d6bTFvZ2xtIn0.PP7AoUakMfeqdXFHdsY0oA


For author

  • Cite changes under the HEAD tag in CHANGELOG.md
  • PR contains only one 15x15 icon
  • Changes fall in accordance to the Maki guidelines
    • Icon geometry is aligned to the pixel grid
    • Icon stays within a 2px trim area
    • Icon uses common geometric building blocks used throughout Maki
    • Icon corners are rounded in either full or half pixel increments
    • Any strokes use 1px
  • Upload the new icon to the Maki icon editor
    • Compare with existing icons
    • Test that stroke/backgrounds/padding work visually

For reviewer

  • Review the interactive map provided to confirm legibility and harmony with existing icons
  • Review each icon variation in an editor, turn on grids and outlines and confirm pixel alignment is maintained to the nearest or half pixel
  • Confirm file requirements stated in Maki guidelines are met
  • Confirm all other changes fall in accordance to the Maki guidelines

@tristen tristen requested a review from willymaps June 4, 2021 18:27
@tristen tristen requested a review from samanpwbb as a code owner June 4, 2021 18:27
@tristen
Copy link
Member Author

tristen commented Jun 11, 2021

@ClareTrainor @kelsey-taylor @melanieimfeld Curious what you think about this one. I'm sort of inclined to iterate on this one a bit more and not include it as part of the 7.0.0 milestone.

@melanieimfeld
Copy link
Contributor

@ClareTrainor @kelsey-taylor @melanieimfeld Curious what you think about this one. I'm sort of inclined to iterate on this one a bit more and not include it as part of the 7.0.0 milestone.

@tristen I wonder if the changes could be as simple as a) eliminating the top of the flagpole or b) simplifying the checker pattern or maybe even removing it entirely?
Screen Shot 2021-06-11 at 1 34 26 PM

@willymaps
Copy link
Contributor

willymaps commented Jun 15, 2021

Iterated on @tristen's nice design:

Link: https://api.mapbox.com/styles/v1/william-davis/ckpy76u1i6yfv18mu9rrp2bjg.html?fresh=true&title=view&access_token=pk.eyJ1Ijoid2lsbGlhbS1kYXZpcyIsImEiOiJja2prYmpvMncwM21pMnJtbjg0NnBjbWduIn0.9n0cpRnRO06AI7oydrZjbw#14.76/40.76341/-73.98443

I think a larger single flag with a little bit of motion translates to a race or racetrack and is a bit more legible. Please let me know your thoughts @kelsey-taylor, @melanieimfeld, @tristen

@tristen
Copy link
Member Author

tristen commented Jun 15, 2021

@willymaps thanks for redrawing the icon! the change to just one flag makes sense to me. I modified it slightly to simplify the number of anchor points and to align the pole ends to the nearest pixel.

@tristen tristen requested review from melanieimfeld and removed request for samanpwbb June 15, 2021 19:48
Copy link
Contributor

@melanieimfeld melanieimfeld left a comment

Choose a reason for hiding this comment

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

@tristen @willymaps I like it! Even the checker pattern is visible. Tested it in this map, where I would have expected to find a racetrack, but didn't 😄

@tristen tristen merged commit b9b403c into main Jun 16, 2021
@tristen tristen deleted the issue-471-racetrack branch June 16, 2021 13:34
@tristen tristen mentioned this pull request Jun 22, 2021
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.

3 participants