-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
Simpler error handling
- Loading branch information
There are no files selected for viewing
This file was deleted.
7 comments
on commit d94a974
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was the magic I was talking about. Seems fine to me but I still feel strongly that this should be fixed with a proper loader element. Is charts the only consumer of this new loader style? If so, I think it's fine to compartmentalize it into an element just in this repository but don't worry about that (but feel free to tackle that if you want).
Let's see what @ebidel thinks about it after you open a PR.
From what I can tell, this looks good for firing errors on library loader issues and drawing issues?
You should add in some tests and make sure the existing ones still behave as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wesalvaro Thanks for quick review. I'll make a PR with fixing some issues you pointed out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well..., I've tried to write some test codes for handling loading error with sinon's useFakeServer(), but it seems that it can only stubs XHR responses, but not ones from script tags. Any ideas for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I'm not familiar with how one could go about testing that, sorry. I don't think it's super important as loading is all that we really care about and manual testing seems straightforward.
A new bug has cropped up for my team wrt calling load multiple times via the old loader. I'm going to wrap the current loading stuff together to address the issue, so hang on a bit, if you would, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I can stay here as long as you like. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've got the new loader element checked in but I didn't port any of your changes into it. If you want to integrate them into head now, that would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's awesome! I'll take a look at the new loader.
takes no argument