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 preset for man_made=quay #810

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Conversation

jdhoek
Copy link
Contributor

@jdhoek jdhoek commented Mar 9, 2023

If this PR is otherwise acceptable I can see about making an icon for it.

Also referred to in #529.


I'm having some issues running npm run build; I get the following:

> @openstreetmap/id-tagging-schema@6.0.0 build
> node scripts/build.js


🏗   Validating and building for development...
[…]/id-tagging-schema/data/fields/check_date.json: 
instance.type is not one of enum values: access,address,check,combo,cycleway,defaultCheck,email,lanes,identifier,localized,manyCombo,multiCombo,networkCombo,number,onewayCheck,radio,restrictions,roadspeed,roadheight,semiCombo,structureRadio,tel,textarea,text,typeCombo,url,wikidata,wikipedia

Any idea? That does not seem related to this PR.

@jdhoek jdhoek force-pushed the feature/quay branch 2 times, most recently from 7a30785 to ed9c618 Compare March 9, 2023 20:56
@tyrasd
Copy link
Member

tyrasd commented Mar 10, 2023

I'm having some issues running npm run build; I get the following: […] Any idea?

you most likely haven't run npm install lately? The `schema-builder dependency needs to be updated because the new field types are only defined the newest version.

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 10, 2023

@tyrasd Yeah, that did it. Thanks.

Possible icon submitted here: rapideditor/temaki#83

Screenshot from 2023-03-10 18-53-11

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 10, 2023

The mooring field might be desirable for the man_made=pier preset as well:

https://taginfo.openstreetmap.org/tags/man_made%3Dpier#combinations

Let me know if that should ride along with this PR.

@tyrasd tyrasd added the waitfor-icon a matching icon is missing for this preset label Mar 10, 2023
@tyrasd
Copy link
Member

tyrasd commented Mar 10, 2023

Good idea, let's add the mooring field also to the Pier preset 👍

@tyrasd
Copy link
Member

tyrasd commented Mar 10, 2023

[[note to self: add rendering rule in iD to render side-markers for quays (like retaining wall)]]

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 10, 2023

Good idea, let's add the mooring field also to the Pier preset +1

Done. Could you have a look at the field values I've suggested? Those seem to be the most sensible and well-documented ones.

@tyrasd
Copy link
Member

tyrasd commented Mar 10, 2023

The values for mooring look fine to me: yes/no is of course the most important, and private is reasonably self explaining. I personally (with almost zero sailing/boating experience) wouldn't know how to identify guest/commercial moorings, though. Also, I can't really tell if other values (ferry, declaration, …) would be necessary in some contexts?! 🤷 The wiki (https://wiki.openstreetmap.org/wiki/Key:mooring#Values) is also not super helpful on this.

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 10, 2023

The wiki is indeed a bit vague there.

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 10, 2023

Icon updated to quay; it was merged in temaki.

@tyrasd tyrasd removed the waitfor-icon a matching icon is missing for this preset label Mar 13, 2023
@github-actions
Copy link

🍱 Preview the tagging presets of this pull request here: https://pr-810--ideditor-presets-preview.netlify.app/id/dist/#locale=en.

@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 13, 2023

The icon isn't showing up in the preview, but that's expected I think? (This PR depends on a newer temaki icon release.)

@tyrasd
Copy link
Member

tyrasd commented Mar 13, 2023

yes, as this is brand new, I need to update temaki to v5.3 on iD before it can show up.

tyrasd added a commit to openstreetmap/iD that referenced this pull request Mar 13, 2023
@tyrasd
Copy link
Member

tyrasd commented Mar 13, 2023

after upgrading teamki (and a rebuild/redeploy), the preview has the new icon now. :shipit:

tyrasd added a commit to openstreetmap/iD that referenced this pull request Mar 13, 2023
@jdhoek
Copy link
Contributor Author

jdhoek commented Mar 13, 2023

Yep, there it is. Looks fine to me. 👍

@tyrasd tyrasd merged commit 01578b1 into openstreetmap:main Mar 13, 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.

2 participants