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

Added event for when loading image fails + documentation #5416

Merged
merged 1 commit into from Mar 28, 2017
Merged

Added event for when loading image fails + documentation #5416

merged 1 commit into from Mar 28, 2017

Conversation

Saulzi
Copy link
Contributor

@Saulzi Saulzi commented Mar 27, 2017

Added event for when loading image fails on ImageOverlay layer and added missing documentation for load event

event and existing error handling. Also added missing
documentation for the load event.
@Saulzi
Copy link
Contributor Author

Saulzi commented Mar 27, 2017

This is the same as PR #5415 without the extra documentation file.

Copy link
Member

@perliedman perliedman left a comment

Choose a reason for hiding this comment

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

This looks good, also bonus points for tests. I left some comments for future reference, but I think this is good to merge as it is.

Thanks! 👍

describe('when loaded', function () {
it('should raise the load event', function () {
var loadraised = false;
overlay.once('load', function () { loadraised = true; });
Copy link
Member

Choose a reason for hiding this comment

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

While this works, you can also use sinon to do this and similar tasks in a more explicit fashion, see for example https://github.com/Leaflet/Leaflet/blob/master/spec/suites/core/EventsSpec.js#L20

describe('when load fails', function () {
it('should raise the error event', function () {
var errorRaised = false;
overlay.once('error', function () { errorRaised = true; });
Copy link
Member

Choose a reason for hiding this comment

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

...same here, sinon would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So basically the same as jasmine as you can do jasmine.spy.

Will bear that in mind for any future contributions :)

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.

None yet

3 participants