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

New Bing Maps Styles - AerialWithLabelsOnDemand and RoadOnDemand. #7808

Merged
merged 5 commits into from
May 3, 2019

Conversation

KeyboardSounds
Copy link
Contributor

@KeyboardSounds KeyboardSounds commented May 3, 2019

Adds support for new Bing Maps imagery styles RoadOnDemand and AerialWithLabelsOnDemand. The other versions of these, Road and AerialWithLabels, have been deprecated.

Fixes #7128.

@cesium-concierge
Copy link

Thanks for the pull request @KeyboardSounds!

  • ❌ Missing CLA.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@KeyboardSounds
Copy link
Contributor Author

I work with @kring at Data61, so I'm covered by their CLA.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 3, 2019

Thanks @KeyboardSounds. I added you to our CLA bot.

Could you please also submit a separate issue for someone to remove the deprecated features in a future Cesium release?

@kring
Copy link
Member

kring commented May 3, 2019

Could you please also submit a separate issue for someone to remove the deprecated features in a future Cesium release?

@pjcozzi I don't think there's any real reason to remove AERIAL_WITH_LABELS, etc. in a future release. Bing has deprecated them, but is unlikely to ever remove them completely, and continuing to support them in Cesium is nearly free.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 3, 2019

Gotcha, that is OK with me, thanks.

});

describe('shouldDiscardImage', function() {
fit('does not discard a non-empty image', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

fit snuck into the committed code here, which disables all other tests. (this sort of thing is why fit should just not exist in Jasmine, in my opinion)

Copy link
Contributor

Choose a reason for hiding this comment

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

fit and fdescribe are really helpful if you just need to debug one new test. However, in some of our other repos we added a custom eslint rule to catch it so it would break CI if you committed it. Maybe we should add that to this repo too

Copy link
Contributor

Choose a reason for hiding this comment

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

The spec filtering can run single tests or suites without needing to edit the source, e.g.:

http://localhost:8080/Specs/SpecRunner.html?spec=Core%2FappendForwardSlash%20Appends%20to%20a%20url

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't work if you need to run it from the command line to reproduce an error that only happens in travis

Copy link
Contributor

Choose a reason for hiding this comment

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

our custom web client is honestly crazy

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't our eslint configuraton flag fit/fdescribe? It does everywhere else outside of Cesium.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I'm pretty sure it's just in the eslintrc.json for specific projects

Copy link
Contributor

Choose a reason for hiding this comment

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

Play. We should definitely fix that then, I'll open a PR soon-ish

Copy link
Contributor

Choose a reason for hiding this comment

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

See #7809

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How embarrassing, good catch. 😳

@shunter
Copy link
Contributor

shunter commented May 3, 2019

This all looks good. I may build on this to fix #1353 as well, since the n=z parameter is supported for all imagery types, not just these new ones.

@shunter shunter merged commit 7f34fdd into CesiumGS:master May 3, 2019
@shunter
Copy link
Contributor

shunter commented May 3, 2019

I fixed the few minor things I mentioned and merged this to master. Thanks!

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.

Bing Maps imagerySets AerialWithLabels and Road are deprecated
7 participants