-
Notifications
You must be signed in to change notification settings - Fork 12
Resource Index - Lazy load with turbo frame #587
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
Conversation
d6f68d3 to
b94e342
Compare
| @sortable_fields = Resource::PUBLISHED_KINDS | ||
| if turbo_frame_request? | ||
| per_page = params[:number_of_items_per_page].presence || 25 | ||
| unfiltered = Resource.where(kind: Resource::PUBLISHED_KINDS) # TODO - #FIXME brittle |
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.
FYI 1: i started changing these to unfiltered instead of unpaginated to be more clear
FYI 2: total_entries can't be used anymore now that we're decorating the paginated collection
app/models/resource.rb
Outdated
| scope :sector_names, ->(names) { tag_names(:sectors, names) } | ||
| scope :featured, -> (featured=nil) { featured.present? ? where(featured: featured) : where(featured: true) } | ||
| scope :kind, -> (kind) { where("kind like ?", kind ) } | ||
| scope :kind, ->(kinds) { |
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 so much better!
you didn't want to rename the scope to kinds instead of kind? i've been going back and forth on whether or not i think they should be singular or plural when sending in plural values.
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 yeah, didn't even think of it but gut reaction is plural.
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, esp since we're passing an array here (vs my xxx--xxx--xxx hack), it prob should be plural
| <% if @resources.any? %> | ||
| <div class="grid grid-cols-1 sm:grid-cols-2 lg:grid-cols-3 gap-6"> | ||
| <% @resources.each do |resource| %> | ||
| <%= render "resource_card", resource: resource.decorate %> |
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 we can't use the partial anymore?
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.
nvm. now i see this is to build the skeleton.
|
|
||
| <div class="flex flex-wrap gap-2"> | ||
| <% sortable_fields.each do |sortable_field| %> | ||
| <% Resource::PUBLISHED_KINDS.each do |sortable_field| %> |
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. this is much clearer.
maebeale
left a comment
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 great! this is such a QOL improvement.
What is the goal of this PR and why is this important?
Lazy load resource index and add copy search url
How did you approach the change?
Add turbo_frame
Anything else to add?
screenrecording-2025-12-27_10-55-57.mp4