Fix and improve chat title edit #2285

2025-05-23T20:15:44+08:00

ahatzz11 commented

2025-05-23 20:15:44 +08:00

(Migrated from github.com)

Right now in prod, editing a chat title isn't a valid option from the chat or chat nav - they both result in a "Missing content" error. The existing edit form is also full of extra classes and has some odd behavior.

This PR:

Fixes the ability to edit a title from the chat and chat nav pages
Adds autofocus to the text field so it's immediately selected. This resolves the weird UX where you select 'edit title' but then don't edit the title. Clicking around doesn't cancel the action currently - with autofocus: true it does
Removes some extra classes from the form/text field and tweak a few things to improve the UI

Before (once missing content was fixed) and after:

(edit: the background for the box is now bg-transparent to blend in with the background, so this screenshot is slightly out of date)

Question: Is this data: { turbo_frame: dom_id(chat, "title") }) pattern the correct way to do this? I know the MenuComponent is a custom component we have, I wasn't sure if there is supposed to be something that converts the frame: dom_id(chat, "title") to the turbo_frame within that component or not. This seems like an easy gotcha.

Right now in prod, editing a chat title isn't a valid option from the chat or chat nav - they both result in a "Missing content" error. The existing edit form is also full of extra classes and has some odd behavior. This PR: - Fixes the ability to edit a title from the chat and chat nav pages - Adds autofocus to the text field so it's immediately selected. This resolves the weird UX where you select 'edit title' but then don't edit the title. Clicking around doesn't cancel the action currently - with `autofocus: true` it does - Removes some extra classes from the form/text field and tweak a few things to improve the UI Before (once missing content was fixed) and after: ![CleanShot-003188 2025-05-23 at 07 11 09@2x](https://github.com/user-attachments/assets/c8c36c7d-637f-4172-860c-b07b769957b3) _(edit: the background for the box is now `bg-transparent` to blend in with the background, so this screenshot is slightly out of date)_ **Question:** Is this `data: { turbo_frame: dom_id(chat, "title") })` pattern the correct way to do this? I know the `MenuComponent` is a custom component we have, I wasn't sure if there is supposed to be something that converts the `frame: dom_id(chat, "title")` to the turbo_frame within that component or not. This seems like an easy gotcha.

zachgoll (Migrated from github.com) reviewed 2025-05-23 22:38:45 +08:00

zachgoll (Migrated from github.com) left a comment

So I actually think the problem here is embedded deeper in the menu_item_component.rb. We're exposing a "button-like" API for the item, but not actually processing the frame option at all.

I believe this diff should fix the issue. I didn't adjust any styling, but worth calling out that the title can be edited from both the chat view and the index view (hence the <% bg_class = params[:ctx] == "chat" ? "bg-container" : "bg-container-inset" %> to toggle the background based on that context)

diff --git a/app/components/menu_item_component.rb b/app/components/menu_item_component.rb
index c029afa7..905bfa8d 100644
--- a/app/components/menu_item_component.rb
+++ b/app/components/menu_item_component.rb
@@ -1,9 +1,9 @@
 class MenuItemComponent < ViewComponent::Base
-      button_to href, method: method, class: container_classes, **merged_button_opts, &block
+      button_to href, method: method, class: container_classes, **merged_opts, &block
     elsif variant == :link
-      link_to href, class: container_classes, **opts, &block
+      link_to href, class: container_classes, **merged_opts, &block
     else
       nil
     end
@@ -44,7 +45,7 @@ class MenuItemComponent < ViewComponent::Base
       ].join(" ")
     end
 
-    def merged_button_opts
+    def merged_opts
       merged_opts = opts.dup || {}
       data = merged_opts.delete(:data) || {}
 
@@ -52,6 +53,10 @@ class MenuItemComponent < ViewComponent::Base
         data = data.merge(turbo_confirm: confirm.to_data_attribute)
       end
 
+      if frame.present?
+        data = data.merge(turbo_frame: frame)
+      end
+
       merged_opts.merge(data: data)
     end
 end

So I actually think the problem here is embedded deeper in the `menu_item_component.rb`. We're exposing a "button-like" API for the item, but not actually processing the `frame` option at all. I believe this diff should fix the issue. I didn't adjust any styling, but worth calling out that the title can be edited from both the chat view and the index view (hence the `<% bg_class = params[:ctx] == "chat" ? "bg-container" : "bg-container-inset" %>` to toggle the background based on that context) ```diff diff --git a/app/components/menu_item_component.rb b/app/components/menu_item_component.rb index c029afa7..905bfa8d 100644 --- a/app/components/menu_item_component.rb +++ b/app/components/menu_item_component.rb @@ -1,9 +1,9 @@ class MenuItemComponent < ViewComponent::Base - button_to href, method: method, class: container_classes, **merged_button_opts, &block + button_to href, method: method, class: container_classes, **merged_opts, &block elsif variant == :link - link_to href, class: container_classes, **opts, &block + link_to href, class: container_classes, **merged_opts, &block else nil end @@ -44,7 +45,7 @@ class MenuItemComponent < ViewComponent::Base ].join(" ") end - def merged_button_opts + def merged_opts merged_opts = opts.dup || {} data = merged_opts.delete(:data) || {} @@ -52,6 +53,10 @@ class MenuItemComponent < ViewComponent::Base data = data.merge(turbo_confirm: confirm.to_data_attribute) end + if frame.present? + data = data.merge(turbo_frame: frame) + end + merged_opts.merge(data: data) end end ```

ahatzz11 commented

2025-05-24 03:06:36 +08:00

(Migrated from github.com)

@zachgoll thanks for the diff! I needed to add the frame as a parameter, but that did the trick. I moved the component usage back to frame:

but worth calling out that the title can be edited from both the chat view and the index view

Yeah - I had both in my screenshot. I liked having the singular background for the text but justin clarified it should be more of a blend. I ended up going with bg-transparent instead so it always belnds and there isn't a need to do any matching.

@zachgoll thanks for the diff! I needed to add the `frame` as a parameter, but that did the trick. I moved the component usage back to `frame: ` >but worth calling out that the title can be edited from both the chat view and the index view Yeah - I had both in my screenshot. I liked having the singular background for the text but justin clarified it should be more of a blend. I ended up going with `bg-transparent` instead so it always belnds and there isn't a need to do any matching.

👍 1

zachgoll (Migrated from github.com) approved these changes 2025-05-24 03:30:57 +08:00

zachgoll (Migrated from github.com) left a comment

Ahh, the transparent background works even better! Looks good now.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#2285