Conversation
There was a problem hiding this comment.
Pull request overview
This PR styles the devices page to provide a modern, consistent user experience across both mobile web and desktop. The changes align the devices page with the application's design system, using the "public" layout and implementing responsive design patterns that work well on both web and native mobile platforms.
Changes:
- Applied modern styling to the devices index page with proper spacing, typography, and responsive layout
- Changed DevicesController to use the "public" layout for consistency with other non-account-scoped pages
- Added bridge controller support to the public layout for proper integration with native mobile apps
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| saas/app/views/devices/index.html.erb | Complete redesign of the devices list UI with centered panel layout, improved typography, styled device list items with flexbox layout, and added "Your Fizzy accounts" button that's hidden in native apps |
| saas/app/controllers/devices_controller.rb | Changed to use "public" layout to match other non-account-scoped controllers |
| app/views/layouts/public.html.erb | Added bridge controller support (title, text-size, insets) for proper native app integration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <strong><%= device.name || "Unnamed device" %></strong> | ||
| (<%= device.platform == "apple" ? "iOS" : "Android" %>) | ||
| </span> | ||
| <%= tag.time "Added #{time_ago_in_words(device.created_at)} ago", datetime: device.created_at.to_i, class: "txt-subtle txt-x-small" %> |
There was a problem hiding this comment.
The codebase uses the local_datetime_tag helper for rendering time elements instead of tag.time. This helper ensures consistent formatting and provides JavaScript integration for local time display. The helper is defined in app/helpers/time_helper.rb and is used throughout the application (see app/views/webhooks/_delivery.html.erb:6, app/views/cards/comments/_comment.html.erb:18-19, and many other examples). Consider replacing this with local_datetime_tag(device.created_at, style: :daysago) or a similar appropriate style parameter.
| <%= tag.time "Added #{time_ago_in_words(device.created_at)} ago", datetime: device.created_at.to_i, class: "txt-subtle txt-x-small" %> | |
| <span class="txt-subtle txt-x-small"> | |
| Added <%= local_datetime_tag(device.created_at, style: :daysago) %> | |
| </span> |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <p class="margin-none-block-start">Mobile devices registered with the mobile app will receive push notifications and will be listed here.</p> | ||
| <% end %> | ||
|
|
||
| <%= link_to session_menu_path, class: "btn center txt-small margin-block-start hide-on-native", data: { turbo_prefetch: false } do %> |
There was a problem hiding this comment.
Links to session_menu_path elsewhere in the app typically pass script_name: nil to guarantee the account-picker URL is generated outside any tenant scope. Consider doing the same here to avoid generating a tenanted URL if this page is ever reached with a script_name set.
| <%= link_to session_menu_path, class: "btn center txt-small margin-block-start hide-on-native", data: { turbo_prefetch: false } do %> | |
| <%= link_to session_menu_path(script_name: nil), class: "btn center txt-small margin-block-start hide-on-native", data: { turbo_prefetch: false } do %> |
| </ul> | ||
| <% else %> | ||
| <h1 class="txt-x-large font-weight-black margin-none">No devices registered</h1> | ||
| <p class="margin-none-block-start">Mobile devices registered with the mobile app will receive push notifications and will be listed here.</p> |
There was a problem hiding this comment.
This empty-state copy change will break the existing DevicesControllerTest assertion that looks for a <p> containing “No devices registered”. Either keep that phrase in the <p> (or adjust the markup so the test can assert on a stable selector), or update the test accordingly so CI doesn’t fail.
| <p class="margin-none-block-start">Mobile devices registered with the mobile app will receive push notifications and will be listed here.</p> | |
| <p class="margin-none-block-start">No devices registered. Mobile devices registered with the mobile app will receive push notifications and will be listed here.</p> |
Looks like 👇
In the native apps the header logo and accounts button are hidden.