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

render preview image url in card, when imageUrl is provided #3438

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bikrantjajware
Copy link

@bikrantjajware bikrantjajware commented Oct 28, 2023

Fixes #3144

Updates to Card

  • add Social Preview image of repo to the Card header

Screenshot
Screenshot 2023-10-28 at 6 03 29 PM

@vercel
Copy link

vercel bot commented Oct 28, 2023

@bikrantjajware is attempting to deploy a commit to the github readme stats Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the repo-card Issues related to the pin/repo card. label Oct 28, 2023
Copy link
Collaborator

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

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

Absolutely:

Hey @bikrantjajware, kudos on your pull request. After a thorough review, I've tagged it with hacktoberfest-accepted as it aligns with the specified criteria. To proceed with merging into the main branch, we'll need:

  • An optional query parameter facilitating user-enabled image display.
  • Comprehensive documentation outlining the new feature.
  • Test cases validating the functionality of the introduced feature.

Your attention to these details will help smooth the integration process. Cheers!

@@ -61,6 +61,7 @@ const renderRepoCard = (repo, options = {}) => {
isTemplate,
starCount,
forkCount,
openGraphImageUrl,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you should add this as an optional option to the pin card:

const {
username,
repo,
hide_border,
title_color,
icon_color,
text_color,
bg_color,
theme,
show_owner,
cache_seconds,
locale,
border_radius,
border_color,
} = req.query;

@github-actions github-actions bot added the documentation Improvements or additions to documentation. label Nov 2, 2023
@bikrantjajware
Copy link
Author

Hello @rickstaa ,
thanks for the suggestions. Addressed the comments and added tests + update documentations. Let me know if i should cover any more test scenarios :)

Copy link
Collaborator

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

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

I don't know about the header formatting since, for some repositories in which a custom social image is set, the styling doesn't look right. We could add a small top margin, but then again, people are likely pinning their repositories and can fix the image header size if they want. @qwerty, what do you think?

Readme Card

[![Readme Card](https://github-readme-stats-git-test-card-image-header-rickstaa.vercel.app/api/pin/?username=anuraghazra&repo=github-readme-stats&show_image=true)](https://github.com/anuraghazra/github-readme-stats)

Readme Card

[![Readme Card](https://github-readme-stats-git-test-card-image-header-rickstaa.vercel.app/api/pin/?username=rickstaa&repo=github-emoji-picker&show_image=true)](https://github.com/anuraghazra/github-readme-stats)

Readme Card

[![Readme Card](https://github-readme-stats-git-test-card-image-header-rickstaa.vercel.app/api/pin/?username=rickstaa&repo=tmux-notify&show_image=true)](https://github.com/anuraghazra/github-readme-stats)

Copy link
Collaborator

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

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

I'm not sure what is happening, but it looks like when deployed, the images don't show up when they are used in markdown, while they show up when the API link is visited directly. Maybe it's a caching problem?

image

@rickstaa
Copy link
Collaborator

rickstaa commented Nov 2, 2023

Hello @rickstaa , thanks for the suggestions. Addressed the comments and added tests + update documentations. Let me know if i should cover any more test scenarios :)

Thanks for addressing my points. I think your pull request is good to be merged. I, however, noticed that it doesn't seem to work on the deployed instance 🤔.

@bikrantjajware
Copy link
Author

Looks like github hides the image behind some proxy (https://github.com/orgs/community/discussions/30326), and probably the proxy service (camo) is unable to read the repo image (maybe timed out error). I'll look into possible solutions
Maybe change in the way we render image in svg

@bikrantjajware
Copy link
Author

Hello @rickstaa , i'm converting the repo_image to a base64 string data and using that to render the header image. Tested with deployed vercel url probably should work now.

@rickstaa
Copy link
Collaborator

rickstaa commented Nov 6, 2023

Thanks a bunch for incorporating the requested tweaks and squashing that bug! The changes look stellar.

GitHub Readme Card](https://github.com/anuraghazra/github-readme-stats)

[![GitHub Readme Card](https://github-readme-stats-wlrp4x0wa-rickstaa.vercel.app/api/pin/?username=anuraghazra&repo=github-readme-stats&show_image=true)](https://github.com/anuraghazra/github-readme-stats)

And the cleanup on the second snippet is on point.

GitHub Readme Card

[![GitHub Readme Card](https://github-readme-stats-wlrp4x0wa-rickstaa.vercel.app/api/pin/?username=rickstaa&repo=tmux-notify&show_image=true)](https://github.com/anuraghazra/github-readme-stats)

With that docstring in place, your code gets a solid thumbs up. The only puzzle piece left is nailing down the styling for repositories rocking custom social images (see the example below). I've tossed the topic over to @qwerty541 for a second opinion 👍🏻.

GitHub Readme Card

[![GitHub Readme Card](https://github-readme-stats-wlrp4x0wa-rickstaa.vercel.app/api/pin/?username=rickstaa&repo=github-emoji-picker&show_image=true)](https://github.com/anuraghazra/github-readme-stats)

@bikrantjajware
Copy link
Author

Thanks @rickstaa :) , for custom images we can set social images in repo > settings > social preview and upload an image

@rickstaa
Copy link
Collaborator

rickstaa commented Nov 7, 2023

Thanks @rickstaa :) , for custom images we can set social images in repo > settings > social preview and upload an image

Hello @bikrantjajware, thank you for your prompt response. I apologize for any confusion in my previous message. While I am familiar with setting a custom image (as demonstrated in example three), my current query pertains to the card styling when users opt for such custom images. Specifically, I've observed a lack of padding at the top. Could you kindly provide clarification on this matter? Your insights would be greatly appreciated. Thank you.

@bikrantjajware
Copy link
Author

Hello @rickstaa ,
do you mean something like this with padding-top, it looks a bit standing out
image

or do you mean like filling the entire card width
image

@rickstaa
Copy link
Collaborator

rickstaa commented Nov 8, 2023

Hello @rickstaa , do you mean something like this with padding-top, it looks a bit standing out image

or do you mean like filling the entire card width image

I meant the first option since the second option would be undoable, given that users can define any social image they like 😅. Let's see which formatting @qwerty541 prefers 👍🏻.

@bikrantjajware
Copy link
Author

oh i see, probably customizing with url params ?
for 2nd option we can set preserveAspectRatio to none on image tag, though it stretches to fill entire width :)

@rickstaa
Copy link
Collaborator

rickstaa commented Nov 8, 2023

oh, i see, probably customizing with url params ? for 2nd option we can set preserveAspectRatio to none on image tag, though it stretches to fill entire width :)

@bikrantjajware, thanks for the fast response. We don't need to add a URL parameter for the top padding. We simply have to decide on what styling we want to apply. The options we now discussed are:

  • Use top padding.
  • Se preserveAspectRatio to none.

Let's wait and see what the other collaborators think about this 👍🏻. Also, please bear in mind that since we are a small team, PRs are merged in the main branch based on the community's interest (see #1935). After that, we typically merge popular pull requests when more than 2 collaborators approve them. It could, therefore, take some time until your PR shows up on the main branch 😅.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation. hacktoberfest-accepted repo-card Issues related to the pin/repo card.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support repository card's Social Preview
2 participants