gregsadetsky / nycnoise

https://nyc-noise.com
17 stars 1 forks source link

Migrate Opengraph meta tags to django-meta #320

Closed litviakk closed 1 day ago

litviakk commented 2 weeks ago

All Opengraph tags seem to be in place

Screen Shot 2024-09-17 at 8 23 23 PM
gregsadetsky commented 2 weeks ago

Amazing thanks a ton!!

Sorry for the slow replies from my side, very busy week. I'll be able to look into it starting next week

Feel free to hardcode the site name in the code, no need to fetch it from the db :)

Would you be able to add a test for the opengraph tags?

litviakk commented 2 weeks ago

Sure! I'm also thinking maybe keeping all settings and hardcoded values in one place might be easier to read (at least there would be logic there, while now they are just spread over two files). Will move them to nycnoise/settings/base.py if no objections

litviakk commented 1 week ago

Can we merge this one, and I will add other changes as follow-ups (I already have a couple of branches requiring a rebase...)

gregsadetsky commented 1 day ago

this is great, thanks!

we do want opengraph to show up on the static pages as well -- could you please make sure they do (you should be able to make a static page on your dev machine, or if you bring the prod db locally, you'll have tons of static pages to test with)

if static pages could have their opengraph title reflect the static page's title, that would be great! (and you could add a test for that as well)

finally, to clean up, could you get rid of the opengraph templatetags (meta_title, meta_description, etc.) and the core/opengraph.html template that won't be used anymore?

thanks again!

litviakk commented 1 day ago

all's done, let's roll! wee-ship

gregsadetsky commented 1 day ago

looking good!

https://www.opengraph.xyz/url/https%3A%2F%2Fnyc-noise.com%2Fgreg

image

https://www.opengraph.xyz/url/https%3A%2F%2Fnyc-noise.com

image