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

onRemove(): only restore original position if set #44

Merged
merged 1 commit into from
Oct 2, 2021

Conversation

ulken
Copy link

@ulken ulken commented Oct 1, 2021

If the marker is never bounced, the original position is never stored.

Trying to set the position to undefined throws an error.

This behavior is a consequence of #42.

If the marker is never bounced, the original position is never stored.

Trying to set the position to `undefined` throws an error.
@maximeh
Copy link
Owner

maximeh commented Oct 2, 2021

Indeed! Good catch, should've seen that at the review of the previous PR.

Wonder if there's any way to do good unit testing here, I don't the javascript world, any advice on some tool/framework that could be used?

@maximeh maximeh merged commit dafc4ac into maximeh:main Oct 2, 2021
@ulken
Copy link
Author

ulken commented Oct 2, 2021

My go-to testing library for unit testing JS is AVA. However, I think it could be a little tricky with the DOM and Leaflet's usage of globals etc. But should definitely be possible. Might have to use something like JSDOM to make it work.

Another possibility, which may be easier to get going with, is to do end-to-end testing with something like Cypress or Playwright.

@ulken ulken deleted the fix-error_on_remove branch October 2, 2021 19:42
@maximeh
Copy link
Owner

maximeh commented Oct 3, 2021

Cool; thanks for the links. AVA looks real nice. For a start, I think snapshot testing/comparison to make sure the marker ends up where it should in all scenarios would be a very good start, I'll make an issue, maybe one of theses days I'll have time for that :/.

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