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

chore: Update logo for dark or light theme #34229

Merged
merged 3 commits into from
May 14, 2022

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented May 4, 2022

GitHub allow you to set a theme, which come in both dark and light flavors.

Sentry's black logo looks horrible on a dark theme, and "pixels matter". This is the current state, on a dark theme:

image

This PR changes the logo to use the purple Sentry logo, which is readable on both white and black backgrounds.

image
image

Note: The first draft of this PR used a media query to select either the white logo for a dark theme, or a dark logo for a light theme. However, it was pointed out to me that some repos render their readme files on package manager sites, and most of those don't change their background correctly for OS theme. Thus switching to a white logo when the OS was set to dark mode would give a white-on-white invisible logo. Since this readme is published to PyPi here, it makes sense to use the purple logo.

mattjohnsonpint added a commit to getsentry/sentry-cocoa that referenced this pull request May 4, 2022
Update logo to use media query so it looks good on both dark and light themes.

See getsentry/sentry#34229 for screenshot of effect, or view readme on this branch after [changing your theme](https://github.com/settings/appearance).
This was referenced May 4, 2022
@mattjohnsonpint
Copy link
Contributor Author

Some discussion on this getsentry/sentry-electron#479 - apparently this approach doesn't work best due to how other package registries render the readme. Going to try another approach.

@mattjohnsonpint mattjohnsonpint marked this pull request as ready for review May 5, 2022 16:03
@mattjohnsonpint
Copy link
Contributor Author

mattjohnsonpint commented May 5, 2022

This repo publishes to PyPi, so we need to use the purple logo instead. See getsentry/sentry-electron#479 (comment) for more details. Thanks.

@mattjohnsonpint mattjohnsonpint enabled auto-merge (squash) May 5, 2022 16:06
@mattjohnsonpint mattjohnsonpint marked this pull request as draft May 5, 2022 21:40
auto-merge was automatically disabled May 5, 2022 21:40

Pull request was converted to draft

@mwarkentin
Copy link
Member

Went down a bit of a rabbit hole on this one previously: #33799

Copy link
Contributor

@taylangocmen taylangocmen left a comment

Choose a reason for hiding this comment

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

Thanks

@mwarkentin
Copy link
Member

@mattjohnsonpint Looks like there's a new way to support both light/dark mode now: https://github.blog/changelog/2022-05-19-specify-theme-context-for-images-in-markdown-beta/

Not sure if that would be handled better by downstream repositories or not.

@mattjohnsonpint
Copy link
Contributor Author

@mwarkentin - That was the approach used in my first draft. It works great when the readme is rendered on GitHub. Unfortunately, most readme files also make their way to package managers and software repositories (PyPi, NPM, Packagist, NuGet, etc.) and very few of those sites render their own background according to the user's theme preferences. For example, this project's readme on PyPi) is always rendered on a white background, regardless of theme settings. If you happen to set your OS for dark theme, then the Sentry logo would render as white by picking up the theme preference, and it would be invisible on the white background.

Hence, why I updated this PR to just use the purple logo.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants