feat: implement mobile responsiveness on all pages#223
feat: implement mobile responsiveness on all pages#223alihosam-dev wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive mobile responsiveness across the application, transforming a desktop-only experience into one that works seamlessly on all screen sizes. The implementation includes a slide-out hamburger navigation menu, responsive typography and spacing, and mobile-optimized table layouts that transform into card-based displays.
Changes:
- Added Stimulus-based mobile menu controller with hamburger button and overlay for navigation
- Applied Tailwind responsive breakpoints (
sm:,md:,lg:) systematically across all page layouts and components - Transformed table components to display as stacked cards on mobile with label/value pairs
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| app/javascript/controllers/mobile_menu_controller.js | New Stimulus controller for hamburger menu toggle, sidebar slide-in/out, and responsive behavior |
| app/views/layouts/application.html.erb | Added mobile menu button, overlay, and controller initialization with conditional rendering for login page |
| app/components/sidebar_component.html.erb | Made sidebar fixed with slide-in animation on mobile, added close-on-click handlers for menu items |
| app/components/shared/index_table_component.html.erb | Duplicated table structure for desktop grid and mobile card layouts with responsive visibility |
| app/views/users/index.html.erb | Applied responsive margins, typography, and button text with mobile abbreviations |
| app/views/regions/index.html.erb | Applied responsive margins, typography, and button text with mobile abbreviations |
| app/views/projects/show.html.erb | Responsive layout with flex wrapping for buttons, adaptive text sizes, and mobile-specific button labels |
| app/views/subprojects/show.html.erb | Responsive margins, typography, and mixed approach to button text sizing |
| app/views/journals/show.html.erb | Responsive layout with stacked buttons and adaptive text sizes |
| app/views/sessions/login.html.erb | Centered login form with responsive padding and full-width mobile layout |
| app/components/projects/projects_card_component.html.erb | Responsive grid columns and text truncation for project cards |
| app/components/projects/log_schema_component.html.erb | Responsive flex layout for title and action button |
| app/components/modal_component.html.erb | Added responsive padding and proper mobile spacing |
| app/components/content_card_component.html.erb | Responsive padding adjustments |
| app/components/users/form_component.html.erb | Responsive wrapping and text sizing for role checkboxes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/views/subprojects/show.html.erb
Outdated
| <span class="text-xs sm:text-base whitespace-nowrap"><%= t(".actions.edit") %></span> | ||
| <% end %> | ||
| <%= render ActionButtonComponent.new(to: new_project_subproject_log_entry_path(@project, @subproject), icon: "add", turbo_stream: true, size: :large, colour: :primary) do %> | ||
| <%= t(".create_log_entry") %> | ||
| <span class="text-xs sm:text-base whitespace-nowrap"><%= t(".create_log_entry") %></span> | ||
| <% end %> | ||
| <%= render ActionButtonComponent.new(to: new_project_subproject_journal_path(@project, @subproject), icon: "add", size: :large, colour: :primary) do %> | ||
| <%= t(".create_journal_entry") %> | ||
| <span class="text-xs sm:text-base whitespace-nowrap"><%= t(".create_journal_entry") %></span> |
There was a problem hiding this comment.
Inconsistent text sizing pattern for mobile button labels. In this file, buttons use text-xs sm:text-base (visible on both mobile and desktop with different sizes), while in other files like projects/show.html.erb and journals/show.html.erb, the pattern is hidden sm:inline for full text and sm:hidden text-xs for abbreviated text. This creates an inconsistent user experience across different pages. Consider standardizing to one approach throughout the application.
app/views/subprojects/show.html.erb
Outdated
| <section class="mx-4 sm:mx-8 my-6 sm:my-10"> | ||
| <div class="flex flex-col gap-4"> | ||
| <div class="flex flex-row justify-between items-start"> | ||
| <div class="flex flex-col gap-4"> |
There was a problem hiding this comment.
The header section is wrapped in a flex flex-col gap-4 div, which makes the title and buttons always stack vertically, even on larger screens where there's plenty of horizontal space. This differs from the pattern used in projects/show.html.erb (line 3) which uses flex flex-col sm:flex-row sm:justify-between sm:items-start gap-4 to place title and buttons side-by-side on larger screens. Consider using the same responsive pattern here for consistency.
| data-action="turbo:submit-end->modal#close"> | ||
|
|
||
| <div class="card bg-base-100 border border-base-300 rounded-xl shadow-xl w-full max-w-xl p-6 max-h-[90vh] flex flex-col"> | ||
| <div class="card bg-base-100 border border-base-300 rounded-xl shadow-xl w-full max-w-xl p-4 sm:p-6 max-h-[90vh] flex flex-col mx-4"> |
There was a problem hiding this comment.
The modal has both mx-4 on the inner div and p-4 on the parent, which could result in excessive horizontal spacing on mobile devices (8px padding from parent + 16px horizontal margin from child = 24px total on each side). The mx-4 class on the modal dialog is redundant since the parent already has p-4 padding that provides spacing from the viewport edges.
| <div class="card bg-base-100 border border-base-300 rounded-xl shadow-xl w-full max-w-xl p-4 sm:p-6 max-h-[90vh] flex flex-col mx-4"> | |
| <div class="card bg-base-100 border border-base-300 rounded-xl shadow-xl w-full max-w-xl p-4 sm:p-6 max-h-[90vh] flex flex-col"> |
| @@ -1,4 +1,6 @@ | |||
| <ul class="menu shadow-lg bg-base-200 w-64 min-h-screen tracking-wide justify-between"> | |||
| <ul | |||
| data-mobile-menu-target="sidebar" | |||
There was a problem hiding this comment.
The sidebar navigation should have proper ARIA landmark attributes. Consider adding role="navigation" and aria-label="Main navigation" to the <ul> element to help screen reader users identify and navigate to the main menu. Additionally, when hidden on mobile, it should have aria-hidden="true" to prevent screen readers from accessing it when it's visually hidden.
| data-mobile-menu-target="sidebar" | |
| data-mobile-menu-target="sidebar" | |
| role="navigation" | |
| aria-label="Main navigation" | |
| aria-hidden="true" |
|
|
||
| <body class="min-h-screen bg-base-100"> | ||
| <div class="flex min-h-screen"> | ||
| <div class="flex min-h-screen" data-controller="mobile-menu"> |
There was a problem hiding this comment.
The mobile menu controller will fail on the login page because the sidebar and overlay targets are conditionally rendered (only when not on login page), but the controller is attached to the parent div which is always present. When the controller tries to access this.sidebarTarget or this.overlayTarget on the login page, it will throw an error because these targets don't exist.
Consider either:
- Moving the
data-controller="mobile-menu"attribute inside the conditional block so it's only present when the menu button and overlay exist - Using optional targets with
hasSidebarTargetchecks before accessing the targets - Adding a condition to only attach the controller when not on the login page
| <div class="flex min-h-screen" data-controller="mobile-menu"> | |
| <div class="flex min-h-screen" <%= 'data-controller="mobile-menu"' unless current_page?(login_path) %>> |
| <div data-index-table-component-target="emptyRow" class="card bg-white rounded-lg shadow-sm <%= "hidden" unless empty? %>"> | ||
| <div class="flex justify-center p-3 items-center"> | ||
| <div class="text-sm text-base-content/40 font-bold"><%= t("shared.index_table_component.no_entries") %></div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div class="space-y-2"> | ||
| <!-- Mobile view: Stacked cards --> | ||
| <div class="md:hidden space-y-3"> | ||
| <% records.each_with_index do |record, index| %> | ||
| <div data-index-table-component-target="row" class="card bg-white rounded-lg shadow-sm"> | ||
| <div class="p-3 grid items-center gap-4" style="grid-template-columns: <%= column_sizes %>"> | ||
| <% columns.each do |column| %> | ||
| <div data-index-table-component-target="row" class="card bg-white rounded-lg shadow-sm p-4"> | ||
| <% columns.each do |column| %> | ||
| <div class="flex flex-col py-2 border-b border-base-200 last:border-0"> | ||
| <div class="text-xs font-semibold text-base-content mb-1"><%= column.header %></div> | ||
| <div class="text-sm text-base-content"><%= render_cell_content(record, column) %></div> | ||
| <% end %> | ||
| </div> | ||
| </div> | ||
| <% end %> | ||
| </div> | ||
| <% end %> | ||
| <div data-index-table-component-target="emptyRow" class="card bg-white rounded-lg shadow-sm <%= "hidden" unless empty? %>"> | ||
| <div class="flex justify-center p-3 items-center"> | ||
| <div class="flex justify-center p-4 items-center"> | ||
| <div class="text-sm text-base-content/40 font-bold"><%= t("shared.index_table_component.no_entries") %></div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The data-index-table-component-target="emptyRow" is duplicated in both desktop and mobile views, but the controller expects a single emptyRow target (it uses this.emptyRowTarget singular). Stimulus will only return the first matching element when using the singular form, which means the empty state will only work correctly in one view (desktop). The mobile empty state will not be properly controlled by the JavaScript filter function. Consider using a single shared empty row element or handling both empty states in the controller.
| </div> | ||
| </div> | ||
| <%= link_to t(".view_project"), project_path(project), class: "text-sm bg-base-300 rounded-md py-2 flex flex-row justify-center items-center" %> | ||
| <%= link_to t(".view_project"), project_path(project), class: "text-sm bg-base-300 hover:bg-base-400 rounded-md py-2 flex flex-row justify-center items-center transition-colors" %> |
There was a problem hiding this comment.
The transition-colors class is added without a corresponding hover: effect already existing on the element. While hover:bg-base-400 is added, the transition-colors class would provide a smoother transition. However, this is only effective if there are color changes to transition. Consider also adding a transition duration class like duration-200 or duration-300 for a more polished hover effect.
app/views/projects/show.html.erb
Outdated
| <span class="sm:hidden text-xs whitespace-nowrap">Edit Project</span> | ||
| <% end %> | ||
|
|
||
| <%= render ActionButtonComponent.new(to: new_project_subproject_path(@project), icon: "add", size: :large, turbo_stream: true, colour: :primary) do %> | ||
| <%= t(".create_subproject") %> | ||
| <span class="hidden sm:inline"><%= t(".create_subproject") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap">Create Subproject</span> | ||
| <% end %> | ||
|
|
||
| <%= render ActionButtonComponent.new(to: project_path(@project), method: :delete, icon: "delete", colour: :error, size: :large, confirm: t("projects.destroy.confirm")) do %> | ||
| <%= t(".actions.delete") %> | ||
| <span class="hidden sm:inline"><%= t(".actions.delete") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap">Delete Project</span> |
There was a problem hiding this comment.
Multiple hardcoded English text strings in mobile button labels: "Edit Project", "Create Subproject", and "Delete Project". The desktop versions use i18n keys (t(".actions.edit"), t(".create_subproject"), t(".actions.delete")), but mobile versions are hardcoded. This creates inconsistent internationalization and these strings will not translate for non-English users. Consider using i18n keys for all button text.
app/views/journals/show.html.erb
Outdated
| <span class="sm:hidden text-xs whitespace-nowrap">Edit Journal</span> | ||
| <% end %> | ||
| <%= render ActionButtonComponent.new(to: project_subproject_journal_path(@project, @subproject, @journal), method: :delete, icon: "delete", colour: :error, size: :large, confirm: t("journals.destroy.confirm")) do %> | ||
| <%= t(".actions.delete") %> | ||
| <span class="hidden sm:inline"><%= t(".actions.delete") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap">Delete Journal</span> |
There was a problem hiding this comment.
Hardcoded English text strings in mobile button labels: "Edit Journal" and "Delete Journal". The desktop versions use i18n keys (t(".actions.edit") and t(".actions.delete")), but mobile versions are hardcoded. This creates inconsistent internationalization and these strings will not translate for non-English users. Consider using i18n keys for all button text.
app/views/subprojects/show.html.erb
Outdated
| <%= render ActionButtonComponent.new(to: project_subproject_path(@project, @subproject), method: :delete, icon: "delete", colour: :error, size: :large, confirm: t("subprojects.destroy.confirm")) do %> | ||
| <%= t(".actions.delete") %> | ||
| <span class="hidden sm:inline"><%= t(".actions.delete") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap">Delete</span> |
There was a problem hiding this comment.
Hardcoded English text "Delete" should be internationalized. The desktop version uses t(".actions.delete") which supports multiple languages, but the mobile abbreviated text is hardcoded. This creates inconsistent internationalization and will not translate for non-English users. Consider using an i18n key for consistency.
| <span class="sm:hidden text-xs whitespace-nowrap">Delete</span> | |
| <span class="sm:hidden text-xs whitespace-nowrap"><%= t(".actions.delete") %></span> |
alihosam-dev
force-pushed
the
alielbadrawy/mobile-responsiveness
branch
from
110d36a to
46af0e0
Compare
|
Needs brakeman 8.0.2 to pass ruby CI check, updated brakeman in PR #224 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| <body class="min-h-screen bg-base-100"> | ||
| <div class="flex min-h-screen"> | ||
| <div class="flex min-h-screen" <%= "data-controller=\"mobile-menu\"".html_safe unless current_page?(login_path) %>> |
There was a problem hiding this comment.
The use of string interpolation with .html_safe for adding data attributes is not idiomatic and unnecessarily complex. Consider using Rails' tag helpers or conditional attribute hashing instead. For example, you could conditionally add the data-controller attribute using a ternary operator within the HTML tag itself, or use content_tag.
| <div class="flex flex-col sm:flex-row sm:justify-between sm:items-start gap-4"> | ||
| <div class="flex flex-col gap-1"> | ||
| <h1 class="text-2xl sm:text-3xl lg:text-4xl font-bold"><%= @subproject.name %></h1> | ||
| <span class="text-base sm:text-lg text-base-content/60"><%= @subproject.description %></span> | ||
| </div> | ||
| <div class="flex flex-row flex-wrap gap-2"> | ||
| <%= render ActionButtonComponent.new(to: project_subproject_path(@project, @subproject), method: :delete, icon: "delete", colour: :error, size: :large, confirm: t("subprojects.destroy.confirm")) do %> | ||
| <%= t(".actions.delete") %> | ||
| <span class="hidden sm:inline"><%= t(".actions.delete") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap"><%= t(".actions.delete") %></span> | ||
| <% end %> | ||
| <%= render ActionButtonComponent.new(to: edit_project_subproject_path(@project, @subproject), icon: "edit", colour: :info, turbo_stream: true, size: :large) do %> | ||
| <%= t(".actions.edit") %> | ||
| <span class="hidden sm:inline"><%= t(".actions.edit") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap"><%= t(".actions.edit") %></span> | ||
| <% end %> | ||
| <%= render ActionButtonComponent.new(to: new_project_subproject_log_entry_path(@project, @subproject), icon: "add", turbo_stream: true, size: :large, colour: :primary) do %> | ||
| <%= t(".create_log_entry") %> | ||
| <span class="hidden sm:inline"><%= t(".create_log_entry") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap"><%= t(".create_log_entry") %></span> | ||
| <% end %> | ||
| <%= render ActionButtonComponent.new(to: new_project_subproject_journal_path(@project, @subproject), icon: "add", size: :large, colour: :primary) do %> | ||
| <%= t(".create_journal_entry") %> | ||
| <span class="hidden sm:inline"><%= t(".create_journal_entry") %></span> | ||
| <span class="sm:hidden text-xs whitespace-nowrap"><%= t(".create_journal_entry") %></span> | ||
| <% end %> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Missing outer container div. The structure is inconsistent with projects/show.html.erb which has an outer flex-col container wrapping both the header/buttons section and the content cards. Without this wrapper, the ContentCard components at lines 26 and 47 are direct children of the section element rather than being properly contained. This should have a structure like projects/show.html.erb with an outer div.flex.flex-col.gap-4 wrapping lines 2-70.
| window.addEventListener("resize", this.handleResize); | ||
| } | ||
|
|
||
| disconnect() { |
There was a problem hiding this comment.
Potential memory leak: if the controller is disconnected while the menu is open, document.body.style.overflow will remain set to "hidden", preventing scrolling on the page. Consider calling this.close() in the disconnect() method to ensure cleanup of body styles when the controller is removed from the DOM.
| disconnect() { | |
| disconnect() { | |
| this.close(); |
| <% records.each_with_index do |record, index| %> | ||
| <div data-index-table-component-target="row" class="card bg-white rounded-lg shadow-sm"> | ||
| <div class="p-3 grid items-center gap-4" style="grid-template-columns: <%= column_sizes %>"> | ||
| <% columns.each do |column| %> | ||
| <div class="text-sm text-base-content"><%= render_cell_content(record, column) %></div> | ||
| <% end %> | ||
| </div> | ||
| </div> | ||
| <% end %> |
There was a problem hiding this comment.
The desktop and mobile views both use data-index-table-component-target="row", which means the existing JavaScript filter will apply to both versions simultaneously. The filter uses inline display styles which can conflict with Tailwind's responsive display classes (hidden md:block and md:hidden). Consider using separate target names (e.g., "desktopRow" and "mobileRow") or ensuring the controller handles responsive duplicates correctly.
There was a problem hiding this comment.
We should not one-shot this PR. It makes it very difficult to review and maintain. We should split the work into multiple PRs, ideally by page, and further by large components where appropriate.
For example, one PR should focus on improving the native responsiveness of the sidebar, likely renamed to the navbar. On desktop it would remain as is, while on mobile it would switch to a dock-style navigation, for example:
https://daisyui.com/components/dock/
Completed
Another PR could focus on styling the table component and improving its mobile responsiveness. In this case, we should split the parent component into two smaller components, one optimized for mobile and one for desktop.
Please also be mindful of avoiding unnecessary Tailwind classes and reusing our existing components where possible.
|
Tailwind also have a |
TL;DR
Implements mobile responsiveness across all pages with hamburger menu and adaptive layouts.
Closes #219
What is this PR trying to achieve?
Makes the app mobile-friendly. Previously desktop-only, now responsive on all screen sizes.
How did you achieve it?
sm:,md:,lg:) to all views/componentsChecklist