-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/codeforafrica/pesayetu/BHfxzyUmJgcoYrqs8TpGhi28Ymbc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good @kilemensi
maybe not entirely related, but how do we plan to handle multi-chart with the current embed structure
Duuude... You just can't give a brother some props, can you? Give me my ✅ already @KobbyMmo! To answer your question, you're 💯 on the mark, we should also handle multi-charts in embed but we can only look into it once #122 is approved and merged. At the moment, my plan is to go with one of:
PS: For SEO, may be we should also augment current |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks good 🚀 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here is some rock music @kilemensi
🪨 🎹
Description
Render the chart image/screenshot server-side and include the necessary SEO information for social media sharing. This eliminates the need to screenshot the charts on client-side.
NOTE: Seems like Vercel has issues with
node-canvas
: Automattic/node-canvas#1394. We may need to deploy this to dokku for server-side image generation to work.Type of change
Screenshots
Checklist: