-
Notifications
You must be signed in to change notification settings - Fork 0
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
Index primary thumbnail id #304
Conversation
Container Scanning Status: ✅ Success
|
6aedc1f
to
740225f
Compare
9a9e243
to
69270ea
Compare
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.
One non-blocking question. You can either look at it or merge, I'm okay either way.
} | ||
end | ||
|
||
defp primary_thumbnail_service_url( |
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 don't know if it's more readable, but I wonder if a bunch of these conditionals are pattern matchable?
Like
defp primary_thumbnail_service_url(%{"thumbnail_id" => [%{"id" => id}], %{"member_ids" => %{id => stuff}})
for the case where there's an id key (this might not be possible, I dunno if you can match like 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.
I tried similar approaches and could not get matches to happen.
defp thumbnail_service_urls(max_thumbnails, image_service_urls, nil) do | ||
# Truncate image service urls to max value | ||
image_service_urls | ||
|> Enum.slice(0, max_thumbnails) |
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.
😂 Yeah I was wondering about this, were we just putting every thumbnail on the page? I guess it was performing.
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.
Very thorough, thank you. I didn't even realize we could have things with multiple thumbnails!
Left requests about comments inline.
4ac0052
to
f89ece3
Compare
Closes #159
Search and show page before
Updating the resource thumbnail
After updating