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

Add dynamic list preview image for using when sharing in Twitter, OpenGraph #6608

Merged
merged 32 commits into from
Jun 9, 2022

Conversation

Vassilis-Boubis
Copy link
Contributor

@Vassilis-Boubis Vassilis-Boubis commented May 30, 2022

Closes #6054

Technical

Testing

Screenshot

Stakeholders

This is a draft with what we have done so far. We have created successfully the image for the social-card and now we need to create the api endpoint

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

This is cool! Placing images to create a larger representation is tricky but it can really get users excited. I provided a few bits of feedback. overlay_covers_over_background() is about 80 lines long which will make it difficult to test and maintain. So, once you are satisfied with the resulting social card, you should think about how to break this into two smaller functions. The first function could get the images and the second could build the social card from those images. This is really great work and I know our users will be excited to see it.

openlibrary/coverstore/code.py Outdated Show resolved Hide resolved
openlibrary/coverstore/code.py Outdated Show resolved Hide resolved
@mekarpeles mekarpeles marked this pull request as ready for review June 6, 2022 18:51
@mekarpeles mekarpeles self-assigned this Jun 6, 2022
@mekarpeles mekarpeles added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Jun 6, 2022
@mekarpeles
Copy link
Member

https://testing.openlibrary.org/people/mekBot/lists/OL104041L/Popular_Books is currently not passing validation https://cards-dev.twitter.com/validator because it times out so we may have to brainstorm better solutions for generating or caching list previews

Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Mek's changes currently on testing

openlibrary/plugins/openlibrary/lists.py Outdated Show resolved Hide resolved
openlibrary/templates/type/list/view_body.html Outdated Show resolved Hide resolved
@mekarpeles
Copy link
Member

Are we still using all these files?
e.g. https://github.com/internetarchive/openlibrary/pull/6608/files#diff-1409e240379880e2a75399dbbef03b88cefdcd5bb39374494ecb54535d1eca09

Let's cleanup / git rm the one's we're not using please :)

openlibrary/coverstore/code.py Outdated Show resolved Hide resolved
openlibrary/coverstore/code.py Outdated Show resolved Hide resolved
@mekarpeles
Copy link
Member

After this PR lands, we should try to move the rending logic / endpoints to coverstore so our webheads are not doing the work. Right now if we move the location of coverstore python file, then the list/preview endpoint can break.

In the future, the og:twitter url or w/e should link directly to covers.openlibrary.org. This is not a blocker for this PR and can be done in a followup.

@mekarpeles mekarpeles assigned cdrini and unassigned mekarpeles Jun 8, 2022
Co-authored-by: Christian Clauss <cclauss@me.com>
@cdrini cdrini force-pushed the social-card branch 2 times, most recently from 180e482 to e356329 Compare June 9, 2022 01:35
@cdrini cdrini changed the title Social card Add dynamic list preview image for using when sharing in Twitter, OpenGraph Jun 9, 2022
@cdrini cdrini force-pushed the social-card branch 3 times, most recently from 1f6c7f0 to fd5b54f Compare June 9, 2022 02:10
Copy link
Collaborator

@cdrini cdrini left a comment

Choose a reason for hiding this comment

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

Alright! I removed those no longer used images and fixed some other small things I noticed. Tested working: https://testing.openlibrary.org/people/mekBot/lists/OL195979L/Dr._Michael_Levin's_Unofficial_Library/preview.png .

I also did a full rebase and squashed some commits to make the history a little tidier :)

Great work @constantinazouni and @Vassilis-Boubis ! @mekarpeles and I decided to implement these last small fixes to make sure you all can focus on your upcoming exams :) Happy studying!

@cdrini cdrini merged commit 053b305 into internetarchive:master Jun 9, 2022
@cclauss
Copy link
Contributor

cclauss commented Jun 9, 2022

Awesome innovation! I bet that this is going to be a popular feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1 Do this week, receiving emails, time sensitive, . [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better Share Preview for Lists & Reading Log (e.g. Twitter Social Card)
5 participants