Skip to content

Hide early gardens, allotments, university#1742

Merged
zerebubuth merged 2 commits intomasterfrom
zerebubuth/1636-hide-early-no-area-garden
Dec 20, 2018
Merged

Hide early gardens, allotments, university#1742
zerebubuth merged 2 commits intomasterfrom
zerebubuth/1636-hide-early-no-area-garden

Conversation

@zerebubuth
Copy link
Copy Markdown
Member

@zerebubuth zerebubuth commented Dec 19, 2018

Hide early point (no area) gardens, universities. Add allotments as a POI type (was already landuse).

Gardens without an area should already have been allocated a min_zoom of 16.

Connects to #1636.

@zerebubuth zerebubuth requested a review from nvkelso December 19, 2018 17:37
Comment thread yaml/pois.yaml Outdated
# allotments
- filter:
landuse: allotments
min_zoom: 16
Copy link
Copy Markdown
Member

@nvkelso nvkelso Dec 19, 2018

Choose a reason for hiding this comment

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

Gardens and allotments are similar, should they range a little in zoom, and exclude the private, no access ones?

  not: { access: [ "private", "no" ] }
min_zoom: { min: [ { max: [ 16, { col: zoom }, *tier6_min_zoom ] }, 17 ] }

(They are different keys – landuse, leisure, so probably makes sense to keep separate blocks. Fine to keep a-z sorted in the YAML.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Updated in f4609b9.

Comment thread yaml/pois.yaml
# university
- filter: {amenity: university}
min_zoom: { min: [ { max: [ { sum: [ { col: zoom }, 2.55 ] }, *tier3_min_zoom ] }, 15 ] }
min_zoom: { min: [ { max: [ { sum: [ { col: zoom }, 2.55 ] }, *tier3_min_zoom ] }, 16 ] }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just noting for review that college was already set to default min_zoom: 16 so no action was needed there.

z, x, y, 'pois', {
'id': 32055218,
'kind': u'allotments',
'min_zoom': 16,
Copy link
Copy Markdown
Member

@nvkelso nvkelso Dec 19, 2018

Choose a reason for hiding this comment

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

Should be large enough test to see it show up in zoom 16. No action needed, as long as the given area is large enough to generate 16 on it's own with proposed changes in min_zoom calculation above.

Copy link
Copy Markdown
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

Minor change about allotment min_zoom, otherwise LGTM.

Copy link
Copy Markdown
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

Please also adjust the garden default (no area) min_zoom to 17 from 16.

Copy link
Copy Markdown
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

All changes made, ready to merge.

@zerebubuth zerebubuth merged commit 09decb5 into master Dec 20, 2018
@zerebubuth zerebubuth deleted the zerebubuth/1636-hide-early-no-area-garden branch December 20, 2018 10:09
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.

2 participants