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

Allow contributors to add points of interest #387

Merged
merged 6 commits into from
Jan 11, 2023
Merged

Allow contributors to add points of interest #387

merged 6 commits into from
Jan 11, 2023

Conversation

ZeLonewolf
Copy link
Member

This PR sets up the structure for contributors to add POI icons with the following scheme:

  • POI icons are black PNGs that are recolored in the style
  • All POIs are contained in a single style layer with selectors to set color, icon, etc

As a demonstrar, this PR includes bars/pubs and hospitals to show how the single selector works. In addition, the source SVGs for the icons are included.

Eventually I'd like to be able to produce the PNGs from SVG at compile time so contributors can deal solely with SVGs.

The initial icons and colors are not great thanks to my limited design skills. Improvements welcome :)
image

Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

We should continue the discussion in #128 about whether we should minimize the number of POI icon colors, as we’ve done with roads, or have POIs occupy the full color spectrum.

style/layer/poi.js Outdated Show resolved Hide resolved
style/layer/poi.js Outdated Show resolved Hide resolved
style/layer/poi.js Outdated Show resolved Hide resolved
style/sdf/health_cross.svg Outdated Show resolved Hide resolved
style/layer/poi.js Outdated Show resolved Hide resolved
style/layer/poi.js Outdated Show resolved Hide resolved
style/layer/poi.js Outdated Show resolved Hide resolved
style/layer/poi.js Outdated Show resolved Hide resolved
style/layer/poi.js Outdated Show resolved Hide resolved
@1ec5 1ec5 mentioned this pull request Jun 18, 2022
8 tasks
@ZeLonewolf ZeLonewolf mentioned this pull request Jun 19, 2022
@ZeLonewolf ZeLonewolf added this to the 1.0.0 milestone Jun 19, 2022
@ZeLonewolf
Copy link
Member Author

I've updated this PR based on feedback received. Some screen shots:

Location: (localhost link)
image

Location: (localhost link)
image

Location: (localhost linkl)
image

@ZeLonewolf ZeLonewolf marked this pull request as ready for review July 11, 2022 01:12
Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Do we need anything in the way of documentation? For example, the contributor’s guide could explain the POI color categories and lay out some initial criteria for POI classes at each zoom level.

icons/poi_p.svg Outdated Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
@ZeLonewolf
Copy link
Member Author

@1ec5 :
Normalizing icon sizes:

image
image
image

@ZeLonewolf
Copy link
Member Author

Do we need anything in the way of documentation? For example, the contributor’s guide could explain the POI color categories and lay out some initial criteria for POI classes at each zoom level.

Yes, will work on adding that.

@ZeLonewolf ZeLonewolf marked this pull request as draft July 15, 2022 19:31
@ZeLonewolf ZeLonewolf marked this pull request as ready for review July 16, 2022 02:06
@ZeLonewolf ZeLonewolf requested a review from 1ec5 July 16, 2022 02:06
@zekefarwell
Copy link
Collaborator

This is looking good. I like the hospital icon, but I find the cross icon for other medical facilities looks inconsistent with it. I think changing the cross to white inside a blue round rect would unify both icons as the same general category.

@ZeLonewolf ZeLonewolf mentioned this pull request Jul 20, 2022
CONTRIBUTING.md Outdated Show resolved Hide resolved
@ZeLonewolf ZeLonewolf marked this pull request as draft July 23, 2022 22:41
src/layer/poi.js Outdated Show resolved Hide resolved
@1ec5 1ec5 mentioned this pull request Dec 24, 2022
@ZeLonewolf
Copy link
Member Author

Blowing the dust off this PR. Current state:

image
image
image
image

I see that @zekefarwell made a suggestion to make the cross icon consistent with the hospital "H" icon, so I'm planning to experiment with that next.

@@ -266,7 +266,7 @@ Additionally, **`refsByWayName`** is an object mapping way names to text that ca

`refsByWayName` only works if there is no `ref` tag and the expression in the `routeConcurrency` function in layer/highway_shield.js includes the `name` property in the image name. The network needs to be listed as an input value that causes the `match` expression to append `name` to the image name.

When using `overrideByRef` or `refsByWayName`, make sure to add a line to the bottom section of this page explaining why it is necessary, as they are only intended for use in special cases.
When using `overrideByRef` or `refsByWayName`, make sure to add a line to the Special Cases section of this page explaining why it is necessary, as they are only intended for use in special cases.
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a bunch of text to the contributing guide, so this section is no longer on the "bottom"

src/layer/poi.js Outdated Show resolved Hide resolved
src/layer/poi.js Show resolved Hide resolved
src/layer/aeroway.js Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
@ZeLonewolf
Copy link
Member Author

Updated screen shot:

image

@ZeLonewolf ZeLonewolf marked this pull request as ready for review January 10, 2023 04:23
CONTRIBUTING.md Outdated Show resolved Hide resolved
src/constants/color.js Outdated Show resolved Hide resolved
src/constants/color.js Outdated Show resolved Hide resolved
src/js/legend_config.js Outdated Show resolved Hide resolved
src/js/legend_config.js Outdated Show resolved Hide resolved
src/layer/poi.js Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
src/layer/poi.js Outdated Show resolved Hide resolved
src/layer/poi.js Show resolved Hide resolved
Copy link
Member

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

We’ve come a long way since June. The selection of POI types is rather absurdly minimalistic, but let’s get this layer into users’ hands to give a sense of what’s possible and iterate further in separate PRs.

@ZeLonewolf
Copy link
Member Author

image

@ZeLonewolf
Copy link
Member Author

image

@ZeLonewolf ZeLonewolf merged commit 522e5c5 into main Jan 11, 2023
@ZeLonewolf ZeLonewolf deleted the poi-setup branch January 11, 2023 03:51
This was referenced Jan 11, 2023
@wmisener wmisener mentioned this pull request Jan 14, 2023
This was referenced Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants