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

Added CallbackPositionProperty #12170

Merged
merged 7 commits into from
Sep 11, 2024
Merged

Added CallbackPositionProperty #12170

merged 7 commits into from
Sep 11, 2024

Conversation

jfayot
Copy link
Contributor

@jfayot jfayot commented Aug 29, 2024

Description

This is a proposal for a CallbackPositionProperty, without making TS expose Property types as generics as mentioned here

Issue number and link

Testing plan

Added specific spec file with 100% coverage on the CallbackPositionProperty class.
Updated PathVizualizer spec file to allow CallbackPositionProperty sub-sampling.
Added a specific sandcastle demo.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jfayot!

✅ We can confirm we have a CLA on file for you.

@ggetz
Copy link
Contributor

ggetz commented Aug 30, 2024

Thanks @jfayot!

We'll review this shortly, likely early next week. If we merge, it will go into the October 1 release.

@jfayot
Copy link
Contributor Author

jfayot commented Sep 5, 2024

This is ready 👀

@ggetz
Copy link
Contributor

ggetz commented Sep 9, 2024

Thank you for your patience @jfayot! I will be able to do a full review tomorrow.

@ggetz ggetz self-assigned this Sep 10, 2024
Copy link
Contributor

@ggetz ggetz left a comment

Choose a reason for hiding this comment

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

This looks great @jfayot! And this is a fantastic Sandcastle example too!

A few small comments, but otherwise the code is looking great. Thanks again for the PR!

Apps/Sandcastle/gallery/Callback Position Property.html Outdated Show resolved Hide resolved
Apps/Sandcastle/gallery/Callback Position Property.html Outdated Show resolved Hide resolved
@jfayot
Copy link
Contributor Author

jfayot commented Sep 10, 2024

Thanks for the review @ggetz ! Updated according to your comments. Should I resolve conflicts ?

@jfayot
Copy link
Contributor Author

jfayot commented Sep 11, 2024

Resolved conflicts, should be ready to merge, but you'd better double check, I'm not a git expert...

@ggetz
Copy link
Contributor

ggetz commented Sep 11, 2024

Looks great! Thanks again @jfayot!

@ggetz ggetz merged commit d8b3e42 into CesiumGS:main Sep 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants