Replaces backslashes with forward slashes in KML files #8533
Conversation
|
Thank you so much for the pull request @sanjeetsuhag! I noticed this is your first pull request and I wanted to say welcome to the Cesium community! The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.
Reviewers, don't forget to make sure that:
|
|
@sanjeetsuhag there is an eslint failure caught by CI. You can run eslint locally with |
|
Also I believe you started this already, but make sure to add an entry to |
| @@ -595,6 +595,7 @@ import WallGraphics from './WallGraphics.js'; | |||
|
|
|||
| var resource; | |||
| if (defined(uriResolver)) { | |||
| href = href.replace(/\\/g, '/'); | |||
There was a problem hiding this comment.
I think it's worth adding a short inline comment explaining why backslashes need to be converted here.
|
Let's add a simple test for this too. It will look similar to |
|
@sanjeetsuhag can you resolve the merge conflicts? You can resolve the merge conflicts locally, or for small conflicts like this you can use the Github UI. |
CHANGES.md
Outdated
| * Reduced Cesium bundle size by avoiding unnecessarily importing `Cesium3DTileset` in `Picking.js` [#8532](https://github.com/AnalyticalGraphicsInc/cesium/pull/8532) | ||
| * Fixed WebGL warning message about `EXT_float_blend` being implicitly enabled. [#8534](https://github.com/AnalyticalGraphicsInc/cesium/pull/8534) | ||
|
|
||
|
|
| var dataSource = new KmlDataSource(options); | ||
| return dataSource.load('Data/KML/backslash.kmz').then(function(source) { | ||
| expect(source).toBe(dataSource); | ||
| expect(source.entities.values[0]._billboard._image._value).toBeDefined(); |
There was a problem hiding this comment.
This test also passes on master so it will need to be tweaked.
I noticed that source.entities.values[0]._billboard._image._value.url is a data uri if it successfully loads the image. You can use isDataUri to check that url is in fact a data uri.
There was a problem hiding this comment.
This issue was resolved by using isDataUri. The test fails on master, but passes on this branch.
| @@ -1,4 +1,4 @@ | |||
| import { ArcType } from '../../Source/Cesium.js'; | |||
| import { ArcType, isDataUri } from '../../Source/Cesium.js'; | |||
There was a problem hiding this comment.
Style: put these imports on separate lines and in alphabetical order
|
Thanks @sanjeetsuhag! |
Fixes #8133