NEAR-Edu / near-certification-tools

2 stars 2 forks source link

Open graph #20

Closed rashaabdulrazzak closed 2 years ago

rashaabdulrazzak commented 2 years ago

until now Twitter inspect like this:

scrnli_2_18_2022_4-14-32 PM

the LinkedIn inspector

scrnli_2_18_2022_4-14-59 PM

the idea I am checking it in a website and it shows the Twitter image as well

scrnli_2_18_2022_4-17-34 PM So I am checking it now the other idea I was trying to get the title and description dynamically for each certificate but I did not know which function I can call then I talked with Catagy and Ozan and we find the right function so I will push the updated function after I check it again. So it is still working process

ryancwalsh commented 2 years ago

@rashaabdulrazzak I'm not sure I understand your screenshots at https://github.com/NEAR-Edu/near-certification-tools/pull/20#issue-1143058489

I like how @ozanisgor labeled his "Before" and "After".

It seems like below the first screenshot you're then showing how LinkedIn and Twitter handle the "share" now that you've coded this branch. I.e. that those are the "After" screenshots.

If that's true, then that seems like this PR is basically finished (once you clean up the stuff I referred to in my earlier comments). Then remove the "draft" label of this PR to indicate that you feel that this is ready to be reviewed for real.

ryancwalsh commented 2 years ago

@rashaabdulrazzak Also, going forward, try to write more helpful commit messages than these, which were the same and neither was very clear about what you wrere doing:

WIP: open graph 7b7e13b
WIP: open graph e08f678

Commit messages are key whenever there are conflicts that need to get resolved. They remind you (or teammates) what each commit was trying to do.

rashaabdulrazzak commented 2 years ago

@ryancwalsh I made the changes but as I mentioned the tokenIdis not defined I record a video to explain to you the problem now https://www.loom.com/share/92736b30277f4a92b982031328048871

ryancwalsh commented 2 years ago

@rashaabdulrazzak Thanks. I thought tokenId used to work on this page / in this file, but maybe it never did. I've reviewed https://nextjs.org/docs/routing/dynamic-routes and still don't see what is wrong. But I'll keep exploring.

ryancwalsh commented 2 years ago

@rashaabdulrazzak Ok I learned something. I could see tokenId populated in the browser console. So it seems like useRouter only works in the client (browser) and not soon enough. That's why we need to use getServerSideProps. I will push a commit soon. Hopefully it will help.

Maybe you can check whether everything works on Twitter and LinkedIn how we hope. If it works, you can submit this PR.

ryancwalsh commented 2 years ago

@rashaabdulrazzak Please take a look at https://github.com/NEAR-Edu/near-certification-tools/commit/cecdc6dec510fdc72d91fab1ecea77e3b2b910fc to see what I meant about things like using a variable for content={pngUrl} instead of redefining the string each time.

rashaabdulrazzak commented 2 years ago

Open graph is working in LinkedIn and Twitter There are 2 types of cards for the Twitter URL here is the preview for the large card: big and here is the preview for the normal one:

scrnli_2_22_2022_3-51-20 PM

So which one do I have to choose?

rashaabdulrazzak commented 2 years ago

@ryancwalsh I made some changes and now everything is working so the problem I was talking about during the meeting and how I solved it is described in this video https://www.loom.com/share/300f5985e0b94ff7a3a2c8d73debfbde

The final results in sharing button:

Twitter Sharing Button LinkedIn Sharing Button

and the final result for validators:

Twitter Validators LinkedIn Validators

Notes:

1- when you checked it you need to replace the baseUrl in pngUrland certificateUrl with the URL of your online localhost 2- to make my localhost online I used localtunnel the other one you mentioned also works fine. 3- the Twitter card display has two options as I mentioned in the older commit so we can choose the other one if it is more likely.