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

feat(gurubu-client): open graph image generator added #122

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

Conversation

hanifisenturk
Copy link
Contributor

Hi @armagandalkiran, @aahmetcakir !

I added the meta tag for previewing links. I also created an endpoint that generates og images by using edge runtime.

At the moment it is not necessary but we can use that endpoint to show the name of the page which we will visit in the future by using params.

And this is the demo preview of my localhost:
image

Copy link

vercel bot commented Feb 11, 2024

@hanifisenturk is attempting to deploy a commit to the Trendyol Gurubu Team Team on Vercel.

A member of the Team first needs to authorize it.

@hanifisenturk
Copy link
Contributor Author

This PR has been created for #108

@hanifisenturk hanifisenturk changed the title feat(app): open graph image generator added feat(gurubu-client): open graph image generator added Feb 11, 2024
Comment on lines 10 to 21
style={{
backgroundColor: "#ffffff",
backgroundSize: "150px 150px",
height: "100%",
width: "100%",
display: "flex",
textAlign: "center",
alignItems: "center",
justifyContent: "center",
flexDirection: "column",
flexWrap: "nowrap",
fontSize: "48px",
Copy link
Member

Choose a reason for hiding this comment

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

it may not be healthy to keep this inline css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi! @mertcanaltin, I am sorry for the late response 🙏 According to this document, at the moment we can use both inline css and tailwindcss(demo). So I created another variable for this to use DRY principle.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I see, I hope it won't be crowded here 🤔 😄

Comment on lines 24 to 40
<svg
width="496"
height="120"
viewBox="0 0 124 30"
fill="none"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M17.3424 16.3333C16.9852 12.227 13.3578 9 8.93564 9C4.27677 9 0.5 12.5817 0.5 17C0.5 21.4183 4.27677 25 8.93564 25H21.5891V16.3333H17.3424Z"
fill="#528BFF"
/>
<ellipse cx="20.8862" cy="15" rx="10.5446" ry="10" fill="#7839EE" />
<path
d="M42.9747 22.752C42.1107 22.752 41.3247 22.56 40.6167 22.176C39.9207 21.78 39.3627 21.21 38.9427 20.466C38.5347 19.71 38.3307 18.792 38.3307 17.712V14.688C38.3307 13.068 38.7867 11.826 39.6987 10.962C40.6107 10.086 41.8467 9.648 43.4067 9.648C44.9547 9.648 46.1487 10.062 46.9887 10.89C47.8407 11.706 48.2667 12.816 48.2667 14.22V14.292H45.9267V14.148C45.9267 13.704 45.8307 13.302 45.6387 12.942C45.4587 12.582 45.1827 12.3 44.8107 12.096C44.4387 11.88 43.9707 11.772 43.4067 11.772C42.5667 11.772 41.9067 12.03 41.4267 12.546C40.9467 13.062 40.7067 13.764 40.7067 14.652V17.748C40.7067 18.624 40.9467 19.332 41.4267 19.872C41.9067 20.4 42.5787 20.664 43.4427 20.664C44.3067 20.664 44.9367 20.436 45.3327 19.98C45.7287 19.524 45.9267 18.948 45.9267 18.252V18.072H42.9387V16.056H48.2667V22.5H46.0707V21.294H45.7467C45.6627 21.498 45.5247 21.714 45.3327 21.942C45.1527 22.17 44.8767 22.362 44.5047 22.518C44.1327 22.674 43.6227 22.752 42.9747 22.752ZM54.0106 22.644C53.3146 22.644 52.7026 22.488 52.1746 22.176C51.6586 21.852 51.2566 21.408 50.9686 20.844C50.6806 20.28 50.5366 19.632 50.5366 18.9V13.572H52.8046V18.72C52.8046 19.392 52.9666 19.896 53.2906 20.232C53.6266 20.568 54.1006 20.736 54.7126 20.736C55.4086 20.736 55.9486 20.508 56.3326 20.052C56.7166 19.584 56.9086 18.936 56.9086 18.108V13.572H59.1766V22.5H56.9446V21.33H56.6206C56.4766 21.63 56.2066 21.924 55.8106 22.212C55.4146 22.5 54.8146 22.644 54.0106 22.644ZM61.7004 22.5V13.572H63.9324V14.58H64.2564C64.3884 14.22 64.6044 13.956 64.9044 13.788C65.2164 13.62 65.5764 13.536 65.9844 13.536H67.0644V15.552H65.9484C65.3724 15.552 64.8984 15.708 64.5264 16.02C64.1544 16.32 63.9684 16.788 63.9684 17.424V22.5H61.7004ZM72.2391 22.644C71.5431 22.644 70.9311 22.488 70.4031 22.176C69.8871 21.852 69.4851 21.408 69.1971 20.844C68.9091 20.28 68.7651 19.632 68.7651 18.9V13.572H71.0331V18.72C71.0331 19.392 71.1951 19.896 71.5191 20.232C71.8551 20.568 72.3291 20.736 72.9411 20.736C73.6371 20.736 74.1771 20.508 74.5611 20.052C74.9451 19.584 75.1371 18.936 75.1371 18.108V13.572H77.4051V22.5H75.1731V21.33H74.8491C74.7051 21.63 74.4351 21.924 74.0391 22.212C73.6431 22.5 73.0431 22.644 72.2391 22.644ZM79.4969 22.5V20.412H81.1529V11.988H79.4969V9.9H85.9769C86.7449 9.9 87.4109 10.032 87.9749 10.296C88.5509 10.548 88.9949 10.914 89.3069 11.394C89.6309 11.862 89.7929 12.426 89.7929 13.086V13.266C89.7929 13.842 89.6849 14.316 89.4689 14.688C89.2529 15.048 88.9949 15.33 88.6949 15.534C88.4069 15.726 88.1309 15.864 87.8669 15.948V16.272C88.1309 16.344 88.4189 16.482 88.7309 16.686C89.0429 16.878 89.3069 17.16 89.5229 17.532C89.7509 17.904 89.8649 18.39 89.8649 18.99V19.17C89.8649 19.866 89.7029 20.466 89.3789 20.97C89.0549 21.462 88.6049 21.84 88.0289 22.104C87.4649 22.368 86.8049 22.5 86.0489 22.5H79.4969ZM83.5289 20.34H85.7609C86.2769 20.34 86.6909 20.214 87.0029 19.962C87.3269 19.71 87.4889 19.35 87.4889 18.882V18.702C87.4889 18.234 87.3329 17.874 87.0209 17.622C86.7089 17.37 86.2889 17.244 85.7609 17.244H83.5289V20.34ZM83.5289 15.084H85.7249C86.2169 15.084 86.6189 14.958 86.9309 14.706C87.2549 14.454 87.4169 14.106 87.4169 13.662V13.482C87.4169 13.026 87.2609 12.678 86.9489 12.438C86.6369 12.186 86.2289 12.06 85.7249 12.06H83.5289V15.084ZM95.2841 22.644C94.5881 22.644 93.9761 22.488 93.4481 22.176C92.9321 21.852 92.5301 21.408 92.2421 20.844C91.9541 20.28 91.8101 19.632 91.8101 18.9V13.572H94.0781V18.72C94.0781 19.392 94.2401 19.896 94.5641 20.232C94.9001 20.568 95.3741 20.736 95.9861 20.736C96.6821 20.736 97.2221 20.508 97.6061 20.052C97.9901 19.584 98.1821 18.936 98.1821 18.108V13.572H100.45V22.5H98.2181V21.33H97.8941C97.7501 21.63 97.4801 21.924 97.0841 22.212C96.6881 22.5 96.0881 22.644 95.2841 22.644Z"
fill="black"
/>
</svg>
Copy link
Member

Choose a reason for hiding this comment

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

this svg is pretty big, let's import it from somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I exported the logo with text as png format, I removed this whole messy svg data :).

Comment on lines 51 to 52
return new Response(`Failed to generate the image`, {
status: 500,
Copy link
Member

Choose a reason for hiding this comment

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

let's discuss here

Is it healthy to return the user directly to 500?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree btw @mertcanaltin , so I changed this a little bit. When something fails, I also return an ImageResponse but without png data, only text.

Normal condition:
Screenshot 2024-02-18 at 12 07 38

In error condition:
Screenshot 2024-02-18 at 12 07 21

Copy link
Member

Choose a reason for hiding this comment

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

@armagandalkiran @aahmetcakir @hanifisenturk Should we show the cause of the error here?

(
<div style={imageStyle}>
{imageData && (
<img width={372} height={90} src={imageData as unknown as string} />
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could make this place cleaner.

src={imageData as unknown as string}

Comment on lines 27 to 29
const imageData = await fetch(
new URL("../../../../public/logo-with-text.png", import.meta.url)
).then((res) => res.arrayBuffer());
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to take it as an ordinary picture?

Copy link
Contributor Author

@hanifisenturk hanifisenturk Feb 20, 2024

Choose a reason for hiding this comment

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

@mertcanaltin actually I thought that. As I used the edge runtime for this operation, all resources have to get with fetch api. This link explains that as well. Of course, if I used Node runtime, we can get the data via fs.readFile.

@mertcanaltin
Copy link
Member

everything looks great. Thank you @hanifisenturk 🚀
I left a few small comments

@armagandalkiran and @aahmetcakir will check at the appropriate time 🎸

@hanifisenturk
Copy link
Contributor Author

Hi! @armagandalkiran @aahmetcakir @mertcanaltin , it's me, again! 😄

I updated the og logic, removed the API endpoint. At the moment, we are using NextJS's built-in og feature. It generates necessary meta tags for us.

@hanifisenturk hanifisenturk requested a review from mertcanaltin May 3, 2024 19:01
@mertcanaltin
Copy link
Member

Greetings @hanifisenturk , first of all sorry for the late reply, thank you very much for your efforts, I will be reviewing this pr in the future, everything looks great LGTM ;)

<img
width={372}
height={90}
src={"https://gurubu.vercel.app/gurubu-logo.svg"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
src={"https://gurubu.vercel.app/gurubu-logo.svg"}
src={"/gurubu-logo.svg"}

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

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