Sitelet https://web.archive.org/web/20220512141135/https://github.com/github/view_component/pull/1169
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

Readme logo update #1169

Merged
merged 9 commits into from Nov 30, 2021
Merged

Readme logo update #1169

merged 9 commits into from Nov 30, 2021

Conversation

dylanatsmith
Copy link
Collaborator

@dylanatsmith dylanatsmith commented Nov 24, 2021 •

Summary

Takes advantage of GitHub’s newly supported functionality to show/hide images in Markdown depending on which GH theme is served as per user settings.

Further optimises the solution to #1134 shipped in #1143.

Note: There doesn't seem to be a way, at least in GFM, to specify image size. So using exising assets they'll be full-width unless we create smaller ones for this purpose.

@dylanatsmith dylanatsmith requested a review from joelhawksley Nov 24, 2021
@dylanatsmith dylanatsmith self-assigned this Nov 24, 2021
@dylanatsmith dylanatsmith requested a review from as a code owner Nov 24, 2021
@Spone
Copy link
Collaborator

@Spone Spone commented Nov 25, 2021

Great that this feature shipped!

Note: There doesn't seem to be a way, at least in GFM, to specify image size. So using exising assets they'll be full-width unless we create smaller ones for this purpose.

It's a bit large for the README, don't you think?

image

What about changing the width="735" height="84" viewBox="0 0 735 84" values in the existing assets? Since it's SVG there will be no quality loss if people want to use them at larger size in other contexts.

@dylanatsmith
Copy link
Collaborator Author

@dylanatsmith dylanatsmith commented Nov 25, 2021

It's nearly comical on desktop for sure.

Changing the canonical SVG is an option. I don’t think there’d be any side effects. I don’t love the idea of adding those attributes to satisfy this one use case, just on principle, but I’m not opposed.

I think we do one of the following:

  1. Leave it full-width in the readme
  2. Change the size of the canonical SVG
  3. Add additional, readme-specific assets like we’re currently using

I’m leaning toward option 3 at the moment but I’m open.

@BlakeWilliams
Copy link
Member

@BlakeWilliams BlakeWilliams commented Nov 29, 2021

I'm 👍 on option 3, using assets specifically for the README makes sense to me.

@joelhawksley
Copy link
Contributor

@joelhawksley joelhawksley commented Nov 29, 2021

I'm fine with option 3 as well 👍🏻

@joelhawksley joelhawksley merged commit 94163de into github:main Nov 30, 2021
21 checks passed
@dylanatsmith dylanatsmith deleted the readme-logo-update branch Nov 30, 2021
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.

None yet

4 participants