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

Cesium ion logo - support for data URL scheme #9085

Merged
merged 9 commits into from
Aug 10, 2020

Conversation

reginapramesti
Copy link
Contributor

Description of the Problem

In our TerriaJS application, the Cesium ion logo is represented as a data URL because our webpack configuration replaces buildModuleUrl with corresponding require statements - as a result the logo doesn't render. This issue is a result of #8758 which introduces a scheme check on the logo URL. Because TerriaJS doesn't use a http(s) URL scheme, and instead uses the data: scheme for the logo URL, the newly introduced code strips the data: scheme from the URL, and prevents the logo from rendering.

How this PR fixes the Problem

We include the data: scheme check within the if condition in CreditDisplay.js which causes the logo to render appropriately on the TerriaJS application.

Example

This is the data URL used by the TerriaJS application to represent the logo, however when this URL is passed through getDefaultCredit(), the data: is stripped and the logo is stored without a URL scheme. As a result, when loading the logo on the TerriaJS application, the img src looks for a resource on the server, rather than the base64 encoded data.

@cesium-concierge
Copy link

Thank you so much for the pull request @reginapramesti! 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.
  • ❌ Missing CLA.
  • 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.

@reginapramesti
Copy link
Contributor Author

I work for CSIRO's Data61, and I believe I am already covered by their CLA

@kring
Copy link
Member

kring commented Aug 6, 2020

Thanks @reginapramesti! The change makes sense to me. @OmarShehata can you easily confirm that this doesn't reintroduce the WebView logo problem fixed in #8758?

@OmarShehata
Copy link
Contributor

It does seem like a safe change. I'd need to get my cordova re-setup, which I'm happy to do and re-test this in the next few days.

@OmarShehata
Copy link
Contributor

Confirmed Cesium ion logo still works when running with this branch on Android with Cordova and there are no unexpected side effects. Thanks @reginapramesti !

@OmarShehata OmarShehata merged commit 2fc3042 into CesiumGS:master Aug 10, 2020
@reginapramesti reginapramesti deleted the ion-logo-fix branch August 10, 2020 11:17
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.

6 participants