-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Add options.query
to KMLDataSource
#5434
Conversation
Source/DataSources/KmlDataSource.js
Outdated
} | ||
href = proxyUrl(href, proxy, query); |
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.
This still needs to be inside of an if (!hrefResolved)
block, so adjust the original logic to handle that. Basically, if hrefResolved
is true at this point, it means that the item is a data or blob uri, which means we can't/shouldn't append query parameters to it.
At first glance, this looks perfect and that was the only comment I had. Also update CHANGES (just edit the CZML line from your last PR). I'll put this through its paces and take a deeper look and let you know if I find any problems. Thanks @omh1280! |
|
</IconStyle>\ | ||
</Style>\ | ||
</Placemark>'; | ||
debugger; |
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.
You forgot to take this out (always try and remember to npm run eslint
before each commit/push)
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.
Fixed, thanks
Awesome work here @omh1280! Thanks. |
Fixes #5408
Pass around a
query
parameter which eventually gets blobbed onto the various uri inKMLDataSource
. I've added like 6 unit tests, but am unconvinced about the thoroughness of this implementation. @mramato please test elsewhere? :)One problem: using
proxyUrl
to appendquery
means thatprocessNetworkLinkQueryString
will be using the already-formattedhref
(LINE). Therefore, it will be used throughout the text-replacement process ofprocessNetworkLinkQueryString
. I don't think this will (ever) be a problem, unless some authentication token contains[bboxWest]
or something.I've also moved a function call out of an
if
block, so now the behavior is thatproxyUrl
is run for every URI. This is ok because if eitherproxy
orquery
areundefined
,proxyUrl
doesn't do anything.Ottavio