-
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
Sort Dropdown Fixes #621
Sort Dropdown Fixes #621
Conversation
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 nitpick on indentation. But otherwise looks good to me.
Should probably close #597 and mark this as a non-draft? I got them mixed up since this one is still a draft.
|
||
DEFAULT_PAGE_SIZE = 10 | ||
|
||
# GET /podcasts | ||
def index | ||
base_query = policy_scope(Podcast).page(params[:page]).per(DEFAULT_PAGE_SIZE).includes(default_feed: :feed_images) | ||
@podcasts = add_sorting(base_query).filter_by_title(params[:q]) | ||
filtered_podcasts = base_query.filter_by_title(params[:q]) | ||
@podcasts = add_sorting(filtered_podcasts) |
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.
Noting this change is functionally equivalent to what was there before. (Calling both add_sorting
and .filter_by_title
on the base query, assigning the result to @podcasts
). But this is fine, if you think it's more readable this way.
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.
The latter way would maybe be easier for me to come back and understand what's happening, but I'm not insistent about it. I can change it back to the way it was before.
app/views/podcasts/index.html.erb
Outdated
<li><%= link_to "# of Episodes", podcasts_path(sort: "episode_count"), class: "dropdown-item" %></li> | ||
<li><%= link_to "A-Z", podcasts_path(sort: "A-Z"), class: "dropdown-item" %></li> | ||
<li><%= link_to "Z-A", podcasts_path(sort: "Z-A"), class: "dropdown-item" %></li> | ||
<li><%= active_link_to "Recent Activity", podcasts_path(sort: "Recent Activity"), active: :exact, class: "dropdown-item text-decoration-none" %></li> |
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 think these should stay indented, inside the <ul>
?
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 good to me!
Closes #597