Moved Viewer functionality to CesiumWidget class
Description
This is a proposal to move Viewer functionalities not related to widgets to CesiumWidget class as proposed in #11967.
Issue number and link
Fixes #11967
Testing plan
- Updated ViewerSpec.js
- Updated CesiumWidgetSpec.js
- Updated Cesium Widget sandcastle
I kept duplicate tests in ViewerSpec, may be some can be removed as they are implemented in CesiumWidgetSpec now?
Author checklist
- [x] I have submitted a Contributor License Agreement
- [x] I have added my name to
CONTRIBUTORS.md - [x] I have updated
CHANGES.mdwith a short summary of my change - [x] I have added or updated unit tests to ensure consistent code coverage
- [x] I have updated the inline documentation, and included code examples where relevant
- [x] I have performed a self-review of my code
Thank you for the pull request, @jfayot!
:white_check_mark: We can confirm we have a CLA on file for you.
@jjspace Could you please take a pass on this PR and see if this is a direction we'd like to go API-wise? Please be mindful of any breaking changes, and deprecate or preserve functionality where we can.
@jjspace any update on this one? 👀
Hi @jjspace ! I'll be off for one week starting from now with no connectivity at all... I'll catch up on oct 7th See you soon
Started some initial high level testing, on sandcastles like this https://sandcastle.cesium.com/?src=GeoJSON%20simplestyle.html when using this branch should we be able to replace Cesium.Viewer with Cesium.CesiumWidget?
when using this branch should we be able to replace
Cesium.ViewerwithCesium.CesiumWidget
@lukemckinstry The basic answer is yes. After this PR the API for CesiumWidget and Viewer should be the same so they can be exchanged except for where code interacts with the actual widgets that are added by the Viewer like the timeline and animation controls.
Swapping it in that one you linked works fine for me, were you seeing an issue? I had tried on a handful of sandcastles already but if you see specific issues please share them so we can take a look.
Swapping it in that one you linked works fine for me, were you seeing an issue? I had tried on a handful of sandcastles already but if you see specific issues please share them so we can take a look.
I did see an issue in that sandcastle, but it is working fine now :+1:, so perhaps I set it up wrong
Hi @jjspace, I'm back :grin: I've addressed most of your comments I think, but there are still some pending points... Tell me what should be the next step!
@jjspace I follow up on the questions you had!
@jfayot and @jjspace A quick reminder to make sure CHANGES.md is updated to reflect any new properties or functionality available on the CesiumWidget class. Thanks!
Awesome, thanks @ggetz!
@jfayot Can you please merge in main to make sure there are no more conflicts? Then update the CHANGES.md as Gabby mentioned. I think most of the changes can probably go under a main bullet point of something like "move Viewer functionality to CesiumWidget to increase usability".
Overall with our previous reviews I think this is really close. I'll take another look once the branch is updated!
@jjspace @ggetz, I've rebased my branch and updated the CHANGES.md. I also tried to improve the way canAnimate is updated in the _onTick using a callback. The test "suspends animation by dataSources if allowed" was duplicated in both Viewer and CesiumWidget in order to test it in both context. Hope this is fine for you!