Skip to content

Conversation

@shenxianpeng
Copy link
Contributor

Trying to replace the current readme-banner which does not have a transparent background, so in the dark theme, it displays a white background.

@shenxianpeng shenxianpeng added the enhancement New feature or request label Aug 24, 2022
@shenxianpeng shenxianpeng requested a review from 2bndy5 August 24, 2022 04:14
@2bndy5
Copy link
Contributor

2bndy5 commented Aug 24, 2022

Did you want it to be the same dimensions? I can help.

@2bndy5
Copy link
Contributor

2bndy5 commented Aug 24, 2022

I think the README is actually doing some manipulation to the image:

<img src="/assets/readme-banner.png" width="355" height="128" alt="" />

@shenxianpeng
Copy link
Contributor Author

@2bndy5
Copy link
Contributor

2bndy5 commented Aug 24, 2022

It does on desktop, but mobile is a little squished.
Screenshot_20220823-215123_GitHub.jpg

@shenxianpeng
Copy link
Contributor Author

shenxianpeng commented Aug 24, 2022

Yes, me too. I'll try to find a better width and height. it should be too wide for mobile.

@2bndy5
Copy link
Contributor

2bndy5 commented Aug 24, 2022

I think the min width used should be 320 (going off of common dimensions listed for portrait mode on various phones - see this site)

Then the height would be dependent on the image's aspect ratio.

@2bndy5
Copy link
Contributor

2bndy5 commented Aug 24, 2022

I think github supports the min-width and min-height style properties. So using the image dimensions 1163x320, the recommended minimum would be

    <img src="/assets/readme-banner.png" min-width="320" min-height="88" alt="cpp-linter_brand_logo" />

@shenxianpeng
Copy link
Contributor Author

shenxianpeng commented Aug 24, 2022

In order to view the result in the local environment, I moved readme-banner.png to the profile folder and remove the assets folder.

I hope the image is displayed as big as possible, so I change to width to 400 and remove height from README.md, it looks well on desktop and mobile.

@2bndy5
Copy link
Contributor

2bndy5 commented Aug 24, 2022

Using my above suggestion, min-width + min-height = constant aspect ratio but using as much space as possible.

Using a strict width limits the largest size the image will be. And without a height specified, the aspect ratio may get distorted.

@shenxianpeng
Copy link
Contributor Author

Updated with your suggestion, it looks great 👍

Copy link
Contributor

@2bndy5 2bndy5 left a comment

Choose a reason for hiding this comment

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

So pretty! 😍

@shenxianpeng shenxianpeng merged commit 97ce944 into main Aug 24, 2022
@shenxianpeng shenxianpeng deleted the update-banner branch August 24, 2022 05:17
@shenxianpeng
Copy link
Contributor Author

shenxianpeng commented Aug 24, 2022

Oh NO, it seems I can not change the refer structure

@2bndy5
Copy link
Contributor

2bndy5 commented Aug 24, 2022

Oh NO, it seems I can not remove the assets folder.

I was just going to say, something didn't work
image

@shenxianpeng
Copy link
Contributor Author

It works now.

@shenxianpeng
Copy link
Contributor Author

I think the current banner on the org overview page is a bit larger than I expected, and users may be drawn to the banner rather than the text/repositories below.
@2bndy5 do you have any suggestions about how to make it smaller? it's okay to be 1/2 or 2/3 of what it is now.

@2bndy5
Copy link
Contributor

2bndy5 commented Sep 2, 2022

You could scale down the image. That's why it was so big to begin with. I needed something that could be scaled down because scaling an image up produced distortion.

2bndy5 added a commit that referenced this pull request Sep 7, 2025
resolves #6

- includes changes to satisfy warnings/errors raised by zizmor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants