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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

png as default favicon to allow alpha transparency #4272

Merged
merged 2 commits into from Jan 14, 2022

Conversation

rbeauchemin
Copy link
Contributor

@rbeauchemin rbeauchemin commented Jan 12, 2022

馃摎 Context

I wanted to have my custom logo as the favicon for my streamlit app, and it loaded in but the transparent parts were filled with a random color. This change makes the default type PNG, a type that allows for alpha transparency and is supported by common browsers (https://caniuse.com/link-icon-png).

  • What kind of change does this PR introduce?

    • Bugfix

馃 Description of Changes

  • Changed type to PNG from JPEG for custom, non-emoji favicons

    • This is a visible (user-facing) change

Revised:
Pretty transparent favicon!

Screen Shot 2022-01-12 at 2 58 34 PM

Current:
Favicon with extra blue stuff around it (alpha filled from common color)
Screen Shot 2022-01-12 at 2 59 48 PM

馃И Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

馃寪 References

This PR has no dependencies.


Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @rbeauchemin!

I'll need to double-check with the team to make sure that there wasn't a specific reason we set the favicon output format to jpeg. This is just me being paranoid, though, and we most likely just chose an arbitrary default when writing this code.

I'll get back to you in a day or two once I've double-checked!

@@ -104,7 +104,7 @@ def set_page_config(
width=-1, # Always use full width for favicons
clamp=False,
channels="RGB",
output_format="JPEG",
output_format="PNG",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I think we may want to set output_format="auto" here, which should normally do a conversion to JPEG, but will keep the image as a PNG if it could have an alpha channel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion. I tested "auto" locally and it does work as expected!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to your suggestion in 1280b7b

with ouptut_formatter set to auto
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

馃帀 thanks for making the updates, @rbeauchemin!

We couldn't remember any reason that we chose to always convert favicons to JPEG internally, so I think it's safe to assume that it was an arbitrary choice made when implementing this feature.

This change should land when v1.5 is released in ~2 weeks.

@vdonato vdonato merged commit 3fe96b2 into streamlit:develop Jan 14, 2022
@rbeauchemin
Copy link
Contributor Author

馃帀 thanks for making the updates, @rbeauchemin!

We couldn't remember any reason that we chose to always convert favicons to JPEG internally, so I think it's safe to assume that it was an arbitrary choice made when implementing this feature.

This change should land when v1.5 is released in ~2 weeks.

Sounds good @vdonato. BTW I like your profile pic haha

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.

Support PNG as the page icon
2 participants