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

CreditDisplay.addDefaultCredit can't be shown in the lightbox #6215

Closed
ggetz opened this issue Feb 13, 2018 · 14 comments · Fixed by #11241
Closed

CreditDisplay.addDefaultCredit can't be shown in the lightbox #6215

ggetz opened this issue Feb 13, 2018 · 14 comments · Fixed by #11241
Labels
category - architecture / api good first issue An opportunity for first time contributors type - bug

Comments

@ggetz
Copy link
Contributor

ggetz commented Feb 13, 2018

CreditDisplay.addDefaultCredit doesn't take Credit.showOnScreen into account, but CreditDisplay.addCredit does.

From the forum: https://groups.google.com/forum/#!topic/cesium-dev/3qiLgbwJCZI

@ggetz ggetz added type - bug good first issue An opportunity for first time contributors labels Feb 13, 2018
@hpinkos
Copy link
Contributor

hpinkos commented Feb 13, 2018

This was more-or-less by design, but it shouldn't be too hard to change

@ggetz
Copy link
Contributor Author

ggetz commented Feb 14, 2018

@hpinkos Is there a particular reason not to show additional credits in the lightbox? Someone on the forum was looking to do just that. If it's on purpose, we should at least clarify in the docs.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 14, 2018

We just made the assumption that a default credit is probably something important and should always show on screen. Like I said, I don't think it will be too hard to change it though.

@mramato
Copy link
Contributor

mramato commented Feb 14, 2018

Maybe I'm just confused but what exactly is the difference between addCredit and addDefaultCredit other than the showOnScreen option? I guess I don't understand why we need two functions anymore.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 14, 2018

addCredit is for credits that depend on the view (imagery/terrain credits), and have to be re-added every frame. addDefaultCredit is for credits that are just always there (the cesium logo)

showOnScreen controls whether the credits are shown in the lightbox or at the bottom of the screen.

@mramato
Copy link
Contributor

mramato commented Feb 14, 2018

Our documentation on this is pretty sparse. We should probably consider changing the names of these functions and expanding their documentation (as well as adding better summary documentation to CreditDisplay overall). Also sounds like showOnScreen should be supported in all cases.

I can't imagine end users ever calling addCredit themselves, they would need to call it every frame, right? Perhaps addCreditToNextFrame would make more sense? And addDefaultCredit just becomes addCredit at some point (though we should eventually make this a collection like we do elsewhere).

@ggetz
Copy link
Contributor Author

ggetz commented Feb 14, 2018

If we're cleaning this up, we should probably make creditDisplay a member of scene as well. Right now you have to go through the frameState: viewer.scene.frameState.creditDisplay

@hpinkos
Copy link
Contributor

hpinkos commented Feb 14, 2018

I think it makes sense that CreditDisplay is part of the frameState, I wouldn't move it.

@mramato
Copy link
Contributor

mramato commented Feb 14, 2018

I think it does make sense to expose it at a higher level though, we do this elsewhere. So a getter property like viewer.credits or viewer.creditDisplay makes total sense.

@shunter
Copy link
Contributor

shunter commented Feb 14, 2018

addCredit is public API for people making their own imagery or terrain provider.

@mramato
Copy link
Contributor

mramato commented Feb 14, 2018

@shunter good point, the name is still terrible though :)

@OmarShehata
Copy link
Contributor

Just 👍 here that we should expose addDefaultCredit in the public API. I recently had to do something like this, and not seeing anything in the public API, ended up just grabbing the div and adding what I needed there directly.

@ggetz
Copy link
Contributor Author

ggetz commented Sep 16, 2022

Also reported by @cmcleese in #10789.

@anne-gropler
Copy link
Contributor

anne-gropler commented Apr 4, 2023

Hello,
is this the current recommendation to grab the html of the lightbox and inject the custom credits there? Because this seems like a hack.

I'd like to add some custom credits in the lightbox. I don't need them in the credits container. Neither addCredits nor addDefaultCredits is doing the job. Since this issue is already 5 years old, is there any chance this gets fixed anytime soon?

Moreover, the credit display is only accessible through viewer.scene._frameState, with frameState not being part of the public API.

https://sandcastle.cesium.com/#c=1ZNNa8MwDIb/ivGlLRSbXbc0rE2Ogx0KOxmKYiutqeMEW2npfv2cBEbZCtsOg+1irEdfr2SsWx+JnSyeMbAV83hmBUbbN+JlZHPF9WgXrSewHoPiiwfllZ9yRNToUezqAA1uCQiFDmgslTZ2Di4CjClGMFeeXdd/p4pmGbBDwHql+IGoi/dSTk2FbhupOCMIe6Tk3lUO/FHxPIsdeEaWHCYMAImt1+tMDjzPJOSz5dCQsRpcxPG+SOek/SfqS6yhd/T7Q1RVldhms/nmEH/pAbTWiRVFcUM7hf5f7N8Yk1hZll/PwJc8i3RxmE9uxh5t07WBWB/cXAhJ2CT9hFFWvT4iCR3jMPoQmspfpWbGnpg1qxv/jGkHMSZP3Tu3ta84SJYp/lOqa8FYv38+YUhbG8IOd/nTBIUQmUzm7UxqW1dB+FD5DQ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category - architecture / api good first issue An opportunity for first time contributors type - bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants