-
-
Notifications
You must be signed in to change notification settings - Fork 941
Gallery support #5693
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
base: main
Are you sure you want to change the base?
Gallery support #5693
Conversation
| FROM | ||
| post | ||
| where | ||
| post.url_content_type like 'image/%'; |
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.
It doesnt make sense to duplicate existing post.url values into the gallery table. However we could consider getting rid of post.url entirely, and relying on post_gallery for all links, but that would be awkward for non-image links.
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 is the correct way to think about it. Posts would now not be a single link, but be a list of links, and have a post_link table that references post_id .
It would support not just images, but any kind of links. In the UIs we could add a button for more links after adding one.
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.
Oh sorry, this is code from a pervious revision I forgot to remove.
I'm personally not sold on the utility of allowing multiple non-media links on a post. That feels like the kind of thing better left for the post body.
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.
Agree that it doesn't make too much sense, but IMO we might as well be flexible if the work has to be done anyway.
Columns like embed_video_url, url, url_content_type, thumbnail_url, alt_text all need to get moved to a new post_link table anyway, so it'd be easier to treat all the links equally, rather than do special cases only for image links.
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.
I'm still against it tbh, I'd prefer to keep the post gallery and url separate and I struggle to see a scenario where 20 links to different web pages is anything but spam, but I'll defer to yous on this.
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.
We can still have code in the api which prevents having more than a single link unless media type is image.
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.
OK, so I've done the DB changes to move links out of Post, but I just want to confirm before I do the changes the API that some fairly major API breaks are acceptable here. If I press ahead, a post will look something like this:
{
"post_view": {
"post": {…},
"links": [
{
"url": "example.org"
…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.
Makes sense.
| url character varying(2000) NOT NULL, | ||
| page integer NOT NULL DEFAULT 0, | ||
| alt_text text, | ||
| caption character varying(200), |
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.
Why add caption when theres already alt_text? And if page is for ordering it should have a clearer name. updated seems unnecessary.
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.
A caption is a short title-like piece of text to go along with the image, while alt_text is meant to be a replacement for the visually impaired. Reddit's galleries include captions and always thought they were a neat addition, so included them here.
I'm not happy with page either, but I couldn't come up with anything better. Index, maybe?
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.
Something like index or sort_order. I suppose the caption makes sense, but also alt_text is missing a max length.
We could set content type to |
Closes #981.
Galleries are a collection of images attached to a post. I wanted to have galleries reside within
Postitself, but I couldn't work out how to do that, so they're just a top-level vec onPostView, basically a copy of how post tags work in #4997.This doesn't introduce any API breaks, but there is some weirdness as
Post's can now have aurl_content_typebut nourl. This is so media filter setting excludes gallery from feeds where that's set.On the AP side, galleries are just federated as multiple attachments. Also,
Imageattachments now havemediaType, to prevent every server having to fetch thecontent_typefor all the images, andsummary, to handle captions.Things left to do:
alt_textin search (ref Include image alt text in post search #5449).