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

always fit to bbox if exists #148

Merged
merged 1 commit into from
Mar 20, 2018
Merged

always fit to bbox if exists #148

merged 1 commit into from
Mar 20, 2018

Conversation

andrewharvey
Copy link
Collaborator

@tristen this undos #30 which was done to address #23, however I can't understand why it's needed?

As a counter example see:
selection_849

  {
      "id": "locality.10733803972294880",
      "type": "Feature",
      "place_type": [
        "locality"
      ],
      "relevance": 1,
      "properties": {
        "wikidata": "Q2733536"
      },
      "text": "Cronulla",
      "place_name": "Cronulla, Sydney, New South Wales, Australia",
      "bbox": [
        151.14313749261,
        -34.07764546406,
        151.16526158076,
        -34.03901257567
      ],
      "center": [
        151.152,
        -34.0574
      ],
      "geometry": {
        "type": "Point",
        "coordinates": [
          151.152,
          -34.0574
        ]
      },
      "context": [
        {
          "id": "postcode.8858496765414730",
          "text": "2230"
        },
        {
          "id": "place.4960085988742460",
          "wikidata": "Q3130",
          "text": "Sydney"
        },
        {
          "id": "region.3703",
          "short_code": "AU-NSW",
          "wikidata": "Q3224",
          "text": "New South Wales"
        },
        {
          "id": "country.3104",
          "short_code": "au",
          "wikidata": "Q408",
          "text": "Australia"
        }
      ]
    }

Currently it won't fit to the bbox since it has 4 context items, instead choosing to zoom in to zoom 16 (default options.zoom) which is far greater than if it fit to the bbox.

The original issue was "1 Broadway, Manhattan, New York, New York 10004, United States" which doesn't return a bbox so would still be fine even with this PR.

@andrewharvey andrewharvey requested a review from tristen March 20, 2018 05:08
Copy link
Member

@tristen tristen left a comment

Choose a reason for hiding this comment

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

Looks like a sensible change! tbh I can't remember why that would have addressed the issue at the time 😛

@andrewharvey andrewharvey merged commit e0cbc93 into master Mar 20, 2018
@andrewharvey andrewharvey deleted the always-fit-to-bbox branch March 20, 2018 21:40
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