576 list podcasts #590
576 list podcasts #590
Conversation
…ink from podcast page index
app/views/podcasts/index.html.erb
Outdated
| </div> | ||
| <%= page_entries_info @podcasts %> | ||
| </div> |
There was a problem hiding this comment.
indents look kinda weird here, is that right?
There was a problem hiding this comment.
Yep, definitely needs some fixes; I'm on it.
app/views/podcasts/index.html.erb
Outdated
| <ul class="dropdown-menu"><a class="dropdown-item active" href="<%= podcasts_path(sort: "Recent") %>">Recent Activity</a></li> | ||
| <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> |
There was a problem hiding this comment.
Could also get these values from the hash in the controller, instead of duplicating them
app/views/podcasts/index.html.erb
Outdated
| <div class="row mt-4 mb-4"> | ||
| <div class="dashboard header"> | ||
| <div class="col d-flex align-items-center "> | ||
| <h1 class="text-black fw-bold me-4 mb-0">My Podcasts</h1> |
There was a problem hiding this comment.
Maybe a design question, but "My" may not be true - this is podcasts they can access, not some kind of implied ownership
There was a problem hiding this comment.
I deferred to the design, but I can change it if you like
There was a problem hiding this comment.
@brandonhundt any preference?
There was a problem hiding this comment.
👍 This looks/works good to me!
I kind of want to see a test for this... but our integration/systems tests in this repo aren't in good shape right now. So that may have to wait. So onwards!
For fallout-tickets, we should maybe open:
- A ticket to i18n-ify these pages (moving all english strings and timestamp/date formatting into
en.yml) - A ticket to support the (optional)
?perquery param - A ticket to fixup N+1 queries. When I load this page locally, I see ~5 SQL queries per podcast displayed on the page. That's fine to start, but we should optimize that so it doesn't need to run a bunch of similar-looking SQL for each podcast.
| # GET /podcasts | ||
| def index | ||
| @podcasts = Podcast.all.limit(10) | ||
| base_query = policy_scope(Podcast).page(params[:page]).per(DEFAULT_PAGE_SIZE) |
There was a problem hiding this comment.
Doesn't need to happen in this PR, but would be nice to also accept the ?per query param here. (Still defaulting to 10, if it's blank).
| <%= link_to "Go to Podcast", podcast_path(podcast), class: "card-link" %> | ||
| </div> | ||
| <div class="card-footer d-grid g-1 lh-sm fs-sm text-end"> | ||
| <strong>Last Saved</strong> |
There was a problem hiding this comment.
Should probably add all these strings (lines 11 thru here) to en.yml. Or open another ticket to do that.
There was a problem hiding this comment.
I can do it as part of this ticket! I was already thinking about it, so might as well just get to it rather than spend the time making another ticket for it.
| </div> | ||
| <div class="card-footer d-grid g-1 lh-sm fs-sm text-end"> | ||
| <strong>Last Saved</strong> | ||
| <datetime><%= podcast.updated_at.strftime("%-m/%-d/%Y, %-l:%M %p") %></datetime> |
There was a problem hiding this comment.
Maybe a @brandonhundt question... but what format is this? And is this something we use throughout the app?
Generally, we should define a :short and :long format for dates and times (4 total), in en.yml. Or maybe a couple more, if we really need to display dates/times differently for some reason. But they should get defined only there, instead of in the views.
|
|
||
| <%= link_to "New podcast", new_podcast_path %> | ||
| <%= paginate @podcasts, window: 2, outer_window: 1 %> |
There was a problem hiding this comment.
Should maybe standardize these window/outer_window configs for all our views. (At some point - doesn't need to happen here).
|
Hey @a-maci29 great work! Its nice to see the sort function working! Now time for one very picky bit of feedback 🎁!!! The class
Thank you!!! |
|
@brandonhundt I pushed up a fix! Go ahead and check it out. :) |
…ring queries on page load
No description provided.