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

Fix credits for multiple viewers #6968

Merged
merged 3 commits into from
Sep 12, 2018
Merged

Fix credits for multiple viewers #6968

merged 3 commits into from
Sep 12, 2018

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Aug 28, 2018

Fixes #6965

We had some some class scoped credits that were causing the same credit instance to be used by the credit display in every viewer. This change clones the credit so each CreditDisplay/IonGeocoderService/IonResource/MapboxImageryProvider has it's own instance of that credit.

@hpinkos hpinkos requested a review from mramato August 28, 2018 16:50
@cesium-concierge
Copy link

Thanks for the pull request @hpinkos!

  • ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

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

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@mramato
Copy link
Contributor

mramato commented Aug 31, 2018

Rather than adding clone and making sure we clone the module scroped credits when we add them, wouldn't it just be way cleaner to turn the module scoped credits into per instance credits? (i.e. create them in the consutrctor instead of at the module level?)

@hpinkos
Copy link
Contributor Author

hpinkos commented Sep 4, 2018

@mramato How is that different than what I'm doing? I'm creating new credit in the constructor of these classes now.

We still need module scoped something because we use these to have the same credit used for multiple classes. Or things like CreditDisplay.cesiumCredit are settable by the end user to swap out the default cesium credit. I guess since we're using all html credits now I could change the module thing to a string instead of a credit instance and call new Credit(moduleScopedCreditString) instead of Credit.clone(moduleScopedCredit), but I'm not sure that's much of a win.

@mramato
Copy link
Contributor

mramato commented Sep 12, 2018

Fair enough, we've got plenty of bigger fish to fry,

@mramato mramato merged commit aa4a877 into master Sep 12, 2018
@mramato mramato deleted the fix-credits branch September 12, 2018 13:56
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.

3 participants