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 self-assigned this Nov 24, 2021
@dylanatsmith dylanatsmith requested a review from a team as a code owner November 24, 2021 21:27
@Spone
Copy link
Collaborator

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

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
Contributor

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

@joelhawksley
Copy link
Member

I'm fine with option 3 as well 👍🏻

@joelhawksley joelhawksley merged commit 94163de into ViewComponent:main Nov 30, 2021
@dylanatsmith dylanatsmith deleted the readme-logo-update branch November 30, 2021 18:32
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