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

Allow local-running Cesium to load its resources #5831

Closed

Conversation

cguinnup
Copy link

Fixes #5830

@cesium-concierge
Copy link

Welcome to the Cesium community @pizza2code!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

@ggetz
Copy link
Contributor

ggetz commented Sep 15, 2017

Hi @pizza2code , thanks for the PR!

So this issue of using the file protocol with Cesium has come up before, see #5326 and #3935

Instead of this change, we'd reccomend using something like Electron to run your app. See our blog post on getting that set up here: https://cesium.com/blog/2016/04/04/an-introduction-to-cesium-desktop-apps-with-electron/

@ggetz
Copy link
Contributor

ggetz commented Sep 18, 2017

@hpinkos Can we consider reopening this?
See #5830 (comment).

Sorry for the additional noise.

Also, per that comment, we can skip the localFile check and just check for xhr.status === 0 in the onload function.

@shunter shunter reopened this Sep 18, 2017
@shunter
Copy link
Contributor

shunter commented Sep 18, 2017

Actually, I think checking for file protocol will still be necessary. From reading the spec, "0" would appear to be a valid error indication for a network-level error for HTTP requests.

@ggetz
Copy link
Contributor

ggetz commented Sep 18, 2017

OK, thanks @shunter !

@mramato
Copy link
Contributor

mramato commented Sep 27, 2017

@pizza2code we can't merge this until you send in a CLA (as noted above). Since this is a small change, if you are unable or don't want to fill out a CLA, I can make the change myself in a different PR. Just let us know either way. Thanks for the PR either way!

@cguinnup
Copy link
Author

cguinnup commented Oct 9, 2017

@mramato Sorry, not sure I'll be able to sign the CLA due to clauses in my employment agreement. You can go ahead and make the change (you're right, it's pretty small) in another PR. :)

@hpinkos
Copy link
Contributor

hpinkos commented Oct 9, 2017

Thanks @pizza2code! I've opened a PR to add the changes here: #5892
Thanks for bringing this up

@hpinkos hpinkos closed this Oct 9, 2017
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.

6 participants