Skip to content

Comments

chore: Update logo for dark or light theme#479

Merged
timfish merged 3 commits intomasterfrom
chore/fix-readme-logo
May 5, 2022
Merged

chore: Update logo for dark or light theme#479
timfish merged 3 commits intomasterfrom
chore/fix-readme-logo

Conversation

@mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented May 4, 2022

Update logo to use media queries 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.

See comments below. Decided to use purple Sentry logo instead, so it renders well both on GitHub and NPM.

#skip-changelog

@timfish
Copy link
Collaborator

timfish commented May 4, 2022

What does this look like when the package is published to npm?

I've messed around with this before but realised that you can end up with the wrong image on npm/pypi etc.

@timfish
Copy link
Collaborator

timfish commented May 4, 2022

Confirmed by pasting the HTML via dev tools with my OS default set to dark:

image

@mattjohnsonpint
Copy link
Contributor Author

Oh. Let me see what can be done about that. I already merged this on a ton of other repos, so hopefully won't have to unwind. 🙂

@timfish
Copy link
Collaborator

timfish commented May 5, 2022

It's tricky and the results vary by package directory. For example, it seems Nuget doesn't display HTML correctly from the readme anyway!

@mattjohnsonpint
Copy link
Contributor Author

@mattjohnsonpint
Copy link
Contributor Author

From testing and research, the built-in support apparently has major problems, and doesn't work when the image is wrapped in a hyperlink. NPM would show both images anyway.

I'm going to leave the default implementation for most of Sentry's repositories, and make adjustments where the repo is used with a package registry. Where media queries aren't supported (like NPM), I'll use Sentry's purple logo (which is better on black background than the black logo, but still not quite as clear as white). Where HTML isn't supported (like Nuget), I'll revert to using plain markdown - which doesn't support centering, but keeps it rendering clean.

@timfish - Thanks for keeping me on my toes! 👍

@mattjohnsonpint mattjohnsonpint marked this pull request as ready for review May 5, 2022 16:24
@mattjohnsonpint mattjohnsonpint marked this pull request as draft May 5, 2022 17:02
@timfish
Copy link
Collaborator

timfish commented May 5, 2022

Thanks for keeping me on my toes!

Yes, sorry. I responded here and then responded in the Rust PR and then noticed all the other PRs too!

@timfish timfish merged commit 1462ed4 into master May 5, 2022
@timfish timfish deleted the chore/fix-readme-logo branch May 5, 2022 23:18
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.

2 participants