Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The layout switch hides existing controls, pagination mishandles out-of-range pages, and the current changes break an existing view spec.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Ports the users index and shared pagination UI to Bootstrap 5.
Changes:
- Applies Bootstrap table/grid styling to users.
- Converts Kaminari pagination to Bootstrap markup.
- Uses the new application layout for users#index.
File summaries
| File | Description |
|---|---|
config/locales/en.yml |
Adds legacy-layout translations. |
app/controllers/users_controller.rb |
Selects the new layout for index. |
app/views/users/index.html.erb |
Adds Bootstrap table and grid styling. |
app/views/kaminari/_paginator.html.erb |
Adds Bootstrap pagination structure. |
app/views/kaminari/_page.html.erb |
Styles page-number items. |
app/views/kaminari/_gap.html.erb |
Styles pagination gaps. |
app/views/kaminari/_first_page.html.erb |
Styles the first-page link. |
app/views/kaminari/_prev_page.html.erb |
Styles the previous-page link. |
app/views/kaminari/_next_page.html.erb |
Styles the next-page link. |
app/views/kaminari/_last_page.html.erb |
Styles the last-page link. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 6
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| <tbody> | ||
| <% @users.each do |user| %> | ||
| <tr> |
There was a problem hiding this comment.
🟡 Changes recommended
The layout switch hides essential controls and feedback, while global pagination changes regress legacy pages and accessibility.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
app/controllers/users_controller.rb:4
- Switching
indexto the default application layout hides functional UI: that layout currently comments out the navigation/session links, page heading, action-bar output, and flash messages. As a result, this page no longer exposes the New User action or sign-out/navigation, and feedback afterdestroyis silently omitted. Port those layout elements before selecting this layout, or keep the legacy layout for this action.
layout "errbit", except: :index
app/views/kaminari/_paginator.html.erb:22
- The previous
current_page.out_of_range?guard was removed. For a request beyond the final page,current_page.last?is false, so the paginator now renders a Next link to an even higher empty page (and a Last link back), rather than suppressing forward navigation. Restore the out-of-range guard around these tags.
<%= next_page_tag unless current_page.last? %>
<%= last_page_tag unless current_page.last? %>
app/views/kaminari/_paginator.html.erb:10
- The pagination landmark lost its accessible name when
aria-label="pager"was removed. Restore anaria-labelso assistive-technology users can distinguish this navigation region from the page's other navigation.
<nav>
app/views/kaminari/_page.html.erb:11
- The active page is indicated only visually by the Bootstrap class. Add
aria-current="page"so screen readers can identify which page is currently selected.
<li class="page-item active">
app/views/kaminari/_gap.html.erb:9
- The truncation marker is now an anchor to
#. Although the parent is visually disabled, the anchor remains keyboard-focusable and activating it can jump the page to the top. Render a non-interactive element with the same Bootstrap class instead.
<%= link_to t("views.pagination.truncate").html_safe, "#", class: "page-link" %>
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
753de7d to
4c7687d
Compare
Changes
Screenshots
kaminari
Users index
TODO