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

Changed type for some properties to Color from Property #9673

Conversation

markchagers
Copy link

@markchagers markchagers commented Jul 10, 2021

Fixes #9667. Some color properties of several Graphics classes (i.e. LabelGraphics.outlineColor) accept only a Cesium.Color, trying to set a ColorMaterialProperty has no effect at runtime. However the typescript definitions specify using a Property. This pull request attempts to address this by changing the type annotations for these properties. Going the other route: changing the implementation to work properly with a ColorMaterialProperty may be a better solution eventually, but is currently beyond my expertise.

@cesium-concierge
Copy link

Thank you so much for the pull request @markchagers! 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.

  • ❌ Missing CONTRIBUTORS.md entry.
  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

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

@markchagers markchagers force-pushed the fix/typescript-definitions-color-members branch from 0356d36 to 764d63f Compare July 10, 2021 16:40
@markchagers
Copy link
Author

Please let me know if there is anything else that needs to be done before this PR can be evaluated. I have not changed or extended any tests because I'm not aware that any of these changes require it.

@srothst1
Copy link
Contributor

@markchagers thank you for the pull request! We will look over this ASAP - I tagged a few other Cesium developers for a review.

Fixes #9667. This pull request stems from the following community forum thread.

@lilleyse
@ebogo1

Copy link
Contributor

@ebogo1 ebogo1 left a comment

Choose a reason for hiding this comment

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

Thanks @markchagers! This looks fine to me, just a couple nits in CHANGES.md.

@mramato Any objections to this change?

CHANGES.md Outdated Show resolved Hide resolved
CHANGES.md Outdated Show resolved Hide resolved
@mramato
Copy link
Contributor

mramato commented Jul 20, 2021

(i.e. LabelGraphics.outlineColor) accept only a Cesium.Color,

This is incorrect. These are Property instances, just not a MaterialProperty, i.e.

graphics.outlineColor = new ConstantProperty(Color.BLUE);
graphics.outlineColor = new SampledProperty(Color);

graphics.outlineColor = Color.BLUE; works but will result in graphics.outlineColor being a ConstantProperty (using implicit conversion borrowed from C#). Because of limitations in TypeScript, we can't tag properties as being multiple types. See this comment for some additional details: #8898 (comment)

If TypeScript has improved and we can better express implicity conversion, that would be awesome.

srothst1 and others added 2 commits July 20, 2021 06:52
Co-authored-by: Eli Bogomolny <31491650+ebogo1@users.noreply.github.com>
Co-authored-by: Eli Bogomolny <31491650+ebogo1@users.noreply.github.com>
@markchagers
Copy link
Author

markchagers commented Jul 22, 2021

@mramato I'm updating the Cesium lib on a project I inherited. In the course of this update I ran into this issue:
this.cesiumEntity.label.fillColor = Cesium.Color.BLUE;
cesiumEntity is a Cesium entity with a LabelGraphics member. In the above line of code the color is updated to a different color. This used to work in Cesium 1.55 (yeah, I know).
This line is flagged by typescript (in VSCode) as incorrect: "Type 'Color' is missing the following properties from type 'Property': ...".
But when I changed the line to:
this.cesiumEntity.label.fillColor = new Cesium.ColorMaterialProperty(Cesium.Color.BLUE);
It didn't work (ColorMaterialProperty was one of the types suggested by VSCode intellisense and judging by the name it seemed a good fit).
Next I tried changing line 21534 in node_modules/cesium/Build/Cesium/Cesium.d.ts to:
fillColor: Color | Property | undefined;
And the result was that I could assign a Color directly and it would work. This led to my assumption the problem was with the typescript definitions.
However, I can confirm that using a ConstantProperty does work, so my pull request is unnecessary.

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.

Typescript errors when setting color properties of various Cesium objects
5 participants