Skip to content

Conversation

@antony
Copy link
Contributor

@antony antony commented Jul 12, 2020

This fixes #379 which was caused by my previous PR not binding the inner method correctly.

I'm still having failures with .flyTo locally but I get these on master anyway so I'm quite sure they're not related to my fix.

I am getting one failure about adding to a container (but it doesn't relate to my change), so just trying to determine if this is something to do with the PR or not.

Opening this for review whilst I do that work.

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • run npm run docs and commit changes to API.md
  • update CHANGELOG.md with changes under master heading before merging

@antony
Copy link
Contributor Author

antony commented Jul 12, 2020

Ok solved. None of my flyTo tests pass, but they don't work with the current master, or the previous release either, so it must just be my machine?

t.end();
});

tt.test('throws if added to an unknown element', (t)=>{
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, I'm not sure what this unit test is for. @scottsfarley93 added it in 7f53ecf#diff-23a1ea5628fe499863040c05a9d338aaR440 but as far as I can tell it's checking to make sure the element is a map, but in that PR at #295 support was added to allow adding the geocoder to a non-map element for a mapless geocoder.

@andrewharvey
Copy link
Collaborator

Thanks @antony and yes don't worry about the flyTo failing test, that's unrelated and will need to be fixed another time.

Copy link
Collaborator

@andrewharvey andrewharvey left a comment

Choose a reason for hiding this comment

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

thanks this looks good to me, and from my manual testing seems to cover all scenarios.

@andrewharvey andrewharvey merged commit 7dc3079 into mapbox:master Jul 12, 2020
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.

fix build failure blocking release

2 participants