From a8c7af5519461ead9c101a5c701d8989897e75a1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20Valim?= <jose.valim@dashbit.co>
Date: Tue, 18 Apr 2023 12:46:54 +0200
Subject: [PATCH] Move star below menu (#1870)

---
 .../live/home_live/session_list_component.ex  |  48 --------
 lib/livebook_web/live/session_live.ex         | 107 ++++++++++--------
 test/livebook_web/live/home_live_test.exs     |  10 +-
 test/livebook_web/live/session_live_test.exs  |   4 +-
 4 files changed, 66 insertions(+), 103 deletions(-)

diff --git a/lib/livebook_web/live/home_live/session_list_component.ex b/lib/livebook_web/live/home_live/session_list_component.ex
index 722404c19..fc9579650 100644
--- a/lib/livebook_web/live/home_live/session_list_component.ex
+++ b/lib/livebook_web/live/home_live/session_list_component.ex
@@ -170,36 +170,6 @@ defmodule LivebookWeb.HomeLive.SessionListComponent do
               <span>Fork</span>
             </button>
           </.menu_item>
-          <span
-            class="tooltip left"
-            data-tooltip={session.file == nil && "Save this notebook before starring it"}
-          >
-            <.menu_item disabled={session.file == nil}>
-              <%= if notebook_starred?(session, @starred_notebooks) do %>
-                <button
-                  type="button"
-                  role="menuitem"
-                  phx-click="unstar_notebook"
-                  phx-target={@myself}
-                  phx-value-id={session.id}
-                >
-                  <.remix_icon icon="star-fill" />
-                  <span>Unstar notebook</span>
-                </button>
-              <% else %>
-                <button
-                  type="button"
-                  role="menuitem"
-                  phx-click="star_notebook"
-                  phx-target={@myself}
-                  phx-value-id={session.id}
-                >
-                  <.remix_icon icon="star-line" />
-                  <span>Star notebook</span>
-                </button>
-              <% end %>
-            </.menu_item>
-          </span>
           <.menu_item>
             <a role="menuitem" href={live_dashboard_process_path(session.pid)} target="_blank">
               <.remix_icon icon="dashboard-2-line" />
@@ -366,18 +336,6 @@ defmodule LivebookWeb.HomeLive.SessionListComponent do
      )}
   end
 
-  def handle_event("star_notebook", %{"id" => session_id}, socket) do
-    session = Enum.find(socket.assigns.sessions, &(&1.id == session_id))
-    Livebook.NotebookManager.add_starred_notebook(session.file, session.notebook_name)
-    {:noreply, socket}
-  end
-
-  def handle_event("unstar_notebook", %{"id" => session_id}, socket) do
-    session = Enum.find(socket.assigns.sessions, &(&1.id == session_id))
-    Livebook.NotebookManager.remove_starred_notebook(session.file)
-    {:noreply, socket}
-  end
-
   def handle_event("disconnect_runtime", %{"id" => session_id}, socket) do
     session = Enum.find(socket.assigns.sessions, &(&1.id == session_id))
     Session.disconnect_runtime(session.pid)
@@ -439,10 +397,4 @@ defmodule LivebookWeb.HomeLive.SessionListComponent do
     JS.dispatch("lb:set_value", to: "#bulk-action-input", detail: %{value: action})
     |> JS.dispatch("submit", to: "#bulk-action-form")
   end
-
-  defp notebook_starred?(%{file: nil} = _session, _starred_notebooks), do: false
-
-  defp notebook_starred?(session, starred_notebooks) do
-    Enum.any?(starred_notebooks, &(&1.file == session.file))
-  end
 end
diff --git a/lib/livebook_web/live/session_live.ex b/lib/livebook_web/live/session_live.ex
index 42cf210ea..45440a43b 100644
--- a/lib/livebook_web/live/session_live.ex
+++ b/lib/livebook_web/live/session_live.ex
@@ -273,24 +273,6 @@ defmodule LivebookWeb.SessionLive do
                     <span>Fork</span>
                   </button>
                 </.menu_item>
-                <span
-                  class="tooltip left"
-                  data-tooltip={@data_view.file == nil && "Save this notebook before starring it"}
-                >
-                  <.menu_item disabled={@data_view.file == nil}>
-                    <%= if @data_view.file in @starred_files do %>
-                      <button type="button" role="menuitem" phx-click="unstar_notebook">
-                        <.remix_icon icon="star-fill" />
-                        <span>Unstar notebook</span>
-                      </button>
-                    <% else %>
-                      <button type="button" role="menuitem" phx-click="star_notebook">
-                        <.remix_icon icon="star-line" />
-                        <span>Star notebook</span>
-                      </button>
-                    <% end %>
-                  </.menu_item>
-                </span>
                 <.menu_item>
                   <a role="menuitem" href={live_dashboard_process_path(@session.pid)} target="_blank">
                     <.remix_icon icon="dashboard-2-line" />
@@ -305,35 +287,66 @@ defmodule LivebookWeb.SessionLive do
                 </.menu_item>
               </.menu>
             </div>
-            <.menu position={:bottom_left} id="notebook-hub-menu">
-              <:toggle>
-                <div
-                  class="inline-flex items-center group cursor-pointer gap-1 mt-1 text-sm text-gray-600 hover:text-gray-800 focus:text-gray-800"
-                  aria-label={@data_view.hub.hub_name}
-                >
-                  <span>in</span>
-                  <span class="text-lg pl-1"><%= @data_view.hub.hub_emoji %></span>
-                  <span><%= @data_view.hub.hub_name %></span>
-                  <.remix_icon icon="arrow-down-s-line" class="invisible group-hover:visible" />
-                </div>
-              </:toggle>
-              <.menu_item :for={hub <- @saved_hubs}>
-                <button
-                  id={"select-hub-#{hub.id}"}
-                  phx-click={JS.push("select_hub", value: %{id: hub.id})}
-                  aria-label={hub.name}
-                  role="menuitem"
-                >
-                  <%= hub.emoji %>
-                  <span class="ml-2"><%= hub.name %></span>
-                </button>
-              </.menu_item>
-              <.menu_item>
-                <.link navigate={~p"/hub"} aria-label="Add Hub" role="menuitem">
-                  <.remix_icon icon="add-line" class="align-middle mr-1" /> Add Hub
-                </.link>
-              </.menu_item>
-            </.menu>
+            <div class="flex flex-nowrap place-content-between items-center gap-2">
+              <.menu position={:bottom_left} id="notebook-hub-menu">
+                <:toggle>
+                  <div
+                    class="inline-flex items-center group cursor-pointer gap-1 mt-1 text-sm text-gray-600 hover:text-gray-800 focus:text-gray-800"
+                    aria-label={@data_view.hub.hub_name}
+                  >
+                    <span>in</span>
+                    <span class="text-lg pl-1"><%= @data_view.hub.hub_emoji %></span>
+                    <span><%= @data_view.hub.hub_name %></span>
+                    <.remix_icon icon="arrow-down-s-line" class="invisible group-hover:visible" />
+                  </div>
+                </:toggle>
+                <.menu_item :for={hub <- @saved_hubs}>
+                  <button
+                    id={"select-hub-#{hub.id}"}
+                    phx-click={JS.push("select_hub", value: %{id: hub.id})}
+                    aria-label={hub.name}
+                    role="menuitem"
+                  >
+                    <%= hub.emoji %>
+                    <span class="ml-2"><%= hub.name %></span>
+                  </button>
+                </.menu_item>
+                <.menu_item>
+                  <.link navigate={~p"/hub"} aria-label="Add Hub" role="menuitem">
+                    <.remix_icon icon="add-line" class="align-middle mr-1" /> Add Hub
+                  </.link>
+                </.menu_item>
+              </.menu>
+
+              <div class="icon-button text-lg pr-[5px]">
+                <%= cond do %>
+                  <% @data_view.file == nil -> %>
+                    <span class="tooltip left" data-tooltip="Save this notebook before starring it">
+                      <.remix_icon icon="star-line" />
+                    </span>
+                  <% @data_view.file in @starred_files -> %>
+                    <button
+                      class="text-yellow-600 tooltip left"
+                      data-tooltip="Unstar notebook"
+                      type="button"
+                      role="menuitem"
+                      phx-click="unstar_notebook"
+                    >
+                      <.remix_icon icon="star-fill" />
+                    </button>
+                  <% true -> %>
+                    <button
+                      class="tooltip left"
+                      data-tooltip="Star notebook"
+                      type="button"
+                      role="menuitem"
+                      phx-click="star_notebook"
+                    >
+                      <.remix_icon icon="star-line" />
+                    </button>
+                <% end %>
+              </div>
+            </div>
           </div>
           <div>
             <.live_component
diff --git a/test/livebook_web/live/home_live_test.exs b/test/livebook_web/live/home_live_test.exs
index f7107a52a..b9bb9363c 100644
--- a/test/livebook_web/live/home_live_test.exs
+++ b/test/livebook_web/live/home_live_test.exs
@@ -150,17 +150,15 @@ defmodule LivebookWeb.HomeLiveTest do
 
       {:ok, view, _} = live(conn, ~p"/")
 
-      view
-      |> element(~s{[data-test-session-id="#{session.id}"] button}, "Star notebook")
-      |> render_click()
+      Livebook.NotebookManager.add_starred_notebook(file, "Special notebook")
+      render(view)
 
       assert view
              |> element(~s/#starred-notebooks/, "Special notebook")
              |> has_element?()
 
-      view
-      |> element(~s{[data-test-session-id="#{session.id}"] button}, "Unstar notebook")
-      |> render_click()
+      Livebook.NotebookManager.remove_starred_notebook(file)
+      render(view)
 
       refute view
              |> element(~s/#starred-notebooks/, "Special notebook")
diff --git a/test/livebook_web/live/session_live_test.exs b/test/livebook_web/live/session_live_test.exs
index e923fb19e..8cf6b9284 100644
--- a/test/livebook_web/live/session_live_test.exs
+++ b/test/livebook_web/live/session_live_test.exs
@@ -710,7 +710,7 @@ defmodule LivebookWeb.SessionLiveTest do
     {:ok, view, _} = live(conn, ~p"/sessions/#{session.id}")
 
     view
-    |> element("button", "Star notebook")
+    |> element("button[data-tooltip=\"Star notebook\"]")
     |> render_click()
 
     assert_receive {:starred_notebooks_updated, starred_notebooks}
@@ -718,7 +718,7 @@ defmodule LivebookWeb.SessionLiveTest do
     assert Enum.any?(starred_notebooks, &(&1.file == file))
 
     view
-    |> element("button", "Unstar notebook")
+    |> element("button[data-tooltip=\"Unstar notebook\"]")
     |> render_click()
 
     assert_receive {:starred_notebooks_updated, starred_notebooks}