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.

Sign up for GitHub

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

Jump to bottom

Set common tags for trees #455

Merged
merged 11 commits into from
May 16, 2022
Merged

Set common tags for trees #455

merged 11 commits into from
May 16, 2022

Conversation

Binnette
Copy link
Contributor

  • Added important fields for trees identification: genus, species, taxon.
  • Added others common fields

@Binnette Binnette mentioned this pull request May 12, 2022
Closed
tordans
tordans requested changes May 13, 2022
View reviewed changes
Copy link
Collaborator

@tordans tordans left a comment

Choose a reason for hiding this comment

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

A few comments. I think we could keep this a bit simpler.

data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
tyrasd
tyrasd requested changes May 13, 2022
View reviewed changes
Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Thanks for the update @Binnette . Please find some further improvement suggestions from my side below.

data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Outdated Show resolved Hide resolved
data/presets/natural/tree.json Show resolved Hide resolved
Binnette
Binnette commented May 13, 2022
View reviewed changes
Copy link
Contributor Author

@Binnette Binnette left a comment

Choose a reason for hiding this comment

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

Seems good to me now 👍

@Binnette Binnette requested a review from tyrasd May 13, 2022 16:44
tyrasd
tyrasd requested changes May 16, 2022
View reviewed changes
Copy link
Member

@tyrasd tyrasd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the amendments. I overlooked earlier that the fields for genus, species and taxon were also missing. It's probably best to use the combo field type for these.

@Binnette
Copy link
Contributor Author

Thanks @tyrasd. I just created the missing files. Please review them 👍

@Binnette Binnette requested a review from tyrasd May 16, 2022 11:16
@tyrasd tyrasd merged commit 628d9b0 into openstreetmap:main May 16, 2022
@Binnette
Copy link
Contributor Author

Great! Thank @tyrasd! Have a nice day 🚀

1ec5
1ec5 reviewed May 17, 2022
View reviewed changes
data/fields/diameter_crown.json Show resolved Hide resolved
data/fields/diameter_crown.json Show resolved Hide resolved
data/presets/natural/tree.json Show resolved Hide resolved
1ec5
1ec5 reviewed May 19, 2022
View reviewed changes
data/fields/genus.json
{
"key": "genus",
"type": "combo",
"label": "Genus",
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this field ever be used for animal species, for example in the Animal Enclosure preset? If so, Vietnamese has distinct words for botanical genera versus zoological genera, so I’ll have to include both in the field name.

Copy link
Member

Choose a reason for hiding this comment

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

Will this field ever be used for animal species

Not in the short or medium term I would say.

@1ec5 1ec5 mentioned this pull request May 28, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1ec5 1ec5 1ec5 left review comments

@tyrasd tyrasd tyrasd approved these changes

@tordans tordans tordans requested changes

Assignees
No one assigned
Labels
add-field new-field
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Binnette @tordans @1ec5 @tyrasd