Improve wording around file system selector (#2157)

Co-authored-by: José Valim <jose.valim@dashbit.co>
This commit is contained in:
Jonatan Kłosko 2023-08-11 11:52:19 +02:00 committed by GitHub
parent 3e940e3034
commit 91253923b2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 62 additions and 26 deletions

View file

@ -185,6 +185,20 @@ defmodule Livebook.Utils do
end)
end
@doc """
Validates a change is not an S3 URL.
"""
@spec validate_not_s3_url(Ecto.Changeset.t(), atom(), String.t()) :: Ecto.Changeset.t()
def validate_not_s3_url(changeset, field, message) do
Ecto.Changeset.validate_change(changeset, field, fn ^field, url ->
if valid_url?(url) and String.starts_with?(url, "s3://") do
[{field, message}]
else
[]
end
end)
end
@doc ~S"""
Validates if the given string forms valid CLI flags.

View file

@ -268,11 +268,14 @@ defmodule LivebookWeb.FileSelectComponent do
<:toggle>
<button
type="button"
class="button-base button-gray button-square-icon"
class="button-base button-gray pl-3 pr-2"
aria-label="switch file system"
disabled={@file_system_select_disabled}
>
<.file_system_icon file_system={@file.file_system} />
<span><%= file_system_name(@file.file_system) %></span>
<div class="pl-0.5 flex items-center">
<.remix_icon icon="arrow-down-s-line" class="text-lg leading-none" />
</div>
</button>
</:toggle>
<%= for file_system <- @file_systems do %>

View file

@ -3,12 +3,20 @@ defmodule LivebookWeb.FileSystemHelpers do
alias Livebook.FileSystem
@doc """
Formats the given file system into a short name.
"""
def file_system_name(file_system)
def file_system_name(%FileSystem.Local{}), do: "Disk"
def file_system_name(%FileSystem.S3{}), do: "S3"
@doc """
Formats the given file system into a descriptive label.
"""
def file_system_label(file_system)
def file_system_label(%FileSystem.Local{}), do: "Local disk"
def file_system_label(%FileSystem.Local{}), do: "Disk"
def file_system_label(%FileSystem.S3{} = fs), do: fs.bucket_url
@doc """

View file

@ -45,7 +45,10 @@ defmodule LivebookWeb.HomeLive do
>
<:topbar_action>
<div class="flex space-x-2">
<.link navigate={~p"/open/file"} class="button-base button-outlined-gray whitespace-nowrap">
<.link
navigate={~p"/open/storage"}
class="button-base button-outlined-gray whitespace-nowrap"
>
Open
</.link>
<.link class="button-base button-blue" patch={~p"/new"}>
@ -63,7 +66,7 @@ defmodule LivebookWeb.HomeLive do
<LayoutHelpers.title text="Home" />
<div class="hidden md:flex space-x-2" role="navigation" aria-label="new notebook">
<.link
navigate={~p"/open/file"}
navigate={~p"/open/storage"}
class="button-base button-outlined-gray whitespace-nowrap"
>
Open

View file

@ -94,7 +94,7 @@ defmodule LivebookWeb.HomeLive.SessionListComponent do
<br />
Looking for unsaved notebooks? <.link
class="font-semibold"
navigate={~p"/open/file?autosave=true"}
navigate={~p"/open/storage?autosave=true"}
phx-no-format
>Browse them here</.link>.
<% end %>

View file

@ -51,9 +51,9 @@ defmodule LivebookWeb.OpenLive do
</div>
<div class="tabs">
<.link patch={~p"/open/file"} class={["tab", @tab == "file" && "active"]}>
<.link patch={~p"/open/storage"} class={["tab", @tab == "storage" && "active"]}>
<.remix_icon icon="file-3-line" class="align-middle" />
<span class="font-medium">From file</span>
<span class="font-medium">From storage</span>
</.link>
<.link patch={~p"/open/url"} class={["tab", @tab == "url" && "active"]}>
<.remix_icon icon="download-cloud-2-line" class="align-middle" />
@ -72,7 +72,7 @@ defmodule LivebookWeb.OpenLive do
<div class="h-96">
<.live_component
:if={@tab == "file"}
:if={@tab == "storage"}
module={LivebookWeb.OpenLive.FileComponent}
id="import-file"
sessions={@sessions}
@ -129,7 +129,7 @@ defmodule LivebookWeb.OpenLive do
<div class="mt-3 text-gray-600 text-sm">
Looking for unsaved notebooks? <.link
class="font-semibold"
navigate={~p"/open/file?autosave=true"}
navigate={~p"/open/storage?autosave=true"}
phx-no-format
>Browse them here</.link>.
</div>
@ -165,7 +165,7 @@ defmodule LivebookWeb.OpenLive do
expanded_path = Path.expand(path)
if File.dir?(expanded_path) do
{:noreply, push_patch(socket, to: ~p"/open/file?path=#{path}")}
{:noreply, push_patch(socket, to: ~p"/open/storage?path=#{path}")}
else
file = FileSystem.File.local(expanded_path)

View file

@ -17,6 +17,10 @@ defmodule LivebookWeb.OpenLive.UrlComponent do
cast({data, types}, attrs, [:url])
|> validate_required([:url])
|> Livebook.Utils.validate_url(:url)
|> Livebook.Utils.validate_not_s3_url(
:url,
~s{invalid s3:// URL scheme, you must first connect to the Cloud Storage in your Hub page and then choose the relevant file in "From storage"}
)
end
@impl true

View file

@ -888,11 +888,11 @@ defmodule LivebookWeb.SessionLive do
<div class="flex flex-col space-y-4">
<div class="tabs">
<.link
patch={~p"/sessions/#{@session.id}/add-file/file"}
class={["tab", @tab == "file" && "active"]}
patch={~p"/sessions/#{@session.id}/add-file/storage"}
class={["tab", @tab == "storage" && "active"]}
>
<.remix_icon icon="file-3-line" class="align-middle" />
<span class="font-medium">From file</span>
<span class="font-medium">From storage</span>
</.link>
<.link
patch={~p"/sessions/#{@session.id}/add-file/url"}
@ -918,7 +918,7 @@ defmodule LivebookWeb.SessionLive do
<div class="grow tab"></div>
</div>
<.live_component
:if={@tab == "file"}
:if={@tab == "storage"}
module={LivebookWeb.SessionLive.AddFileEntryFileComponent}
id="add-file-entry-from-file"
session={@session}

View file

@ -33,6 +33,10 @@ defmodule LivebookWeb.SessionLive.AddFileEntryUrlComponent do
|> validate_required([:url, :name, :copy])
|> Livebook.Notebook.validate_file_entry_name(:name)
|> Livebook.Utils.validate_url(:url)
|> Livebook.Utils.validate_not_s3_url(
:url,
~s{invalid s3:// URL scheme, you must first connect to the Cloud Storage in your Hub page and then choose the relevant file in "From storage"}
)
end
@impl true

View file

@ -135,7 +135,7 @@ defmodule LivebookWeb.SessionLive.FilesListComponent do
<.link
class="w-full flex items-center justify-center p-8 py-1 space-x-2 text-sm font-medium text-gray-500 border border-gray-400 border-dashed rounded-xl hover:bg-gray-100"
role="button"
patch={~p"/sessions/#{@session.id}/add-file/file"}
patch={~p"/sessions/#{@session.id}/add-file/storage"}
>
<.remix_icon icon="add-line" class="text-lg align-center" />
<span>New file</span>

View file

@ -7,7 +7,7 @@ defmodule LivebookWeb.OpenLiveTest do
describe "file selection" do
test "updates the list of files as the input changes", %{conn: conn} do
{:ok, view, _} = live(conn, ~p"/open/file")
{:ok, view, _} = live(conn, ~p"/open/storage")
path = Path.expand("../../../lib", __DIR__) <> "/"
@ -20,7 +20,7 @@ defmodule LivebookWeb.OpenLiveTest do
end
test "allows importing when a notebook file is selected", %{conn: conn} do
{:ok, view, _} = live(conn, ~p"/open/file")
{:ok, view, _} = live(conn, ~p"/open/storage")
path = test_notebook_path("basic")
@ -45,7 +45,7 @@ defmodule LivebookWeb.OpenLiveTest do
@tag :tmp_dir
test "disables import when a directory is selected", %{conn: conn, tmp_dir: tmp_dir} do
{:ok, view, _} = live(conn, ~p"/open/file")
{:ok, view, _} = live(conn, ~p"/open/storage")
view
|> element(~s{form[phx-change="set_path"]})
@ -57,7 +57,7 @@ defmodule LivebookWeb.OpenLiveTest do
end
test "disables import when a nonexistent file is selected", %{conn: conn} do
{:ok, view, _} = live(conn, ~p"/open/file")
{:ok, view, _} = live(conn, ~p"/open/storage")
path = File.cwd!() |> Path.join("nonexistent.livemd")
@ -73,7 +73,7 @@ defmodule LivebookWeb.OpenLiveTest do
@tag :tmp_dir
test "disables open when a write-protected notebook is selected",
%{conn: conn, tmp_dir: tmp_dir} do
{:ok, view, _} = live(conn, ~p"/open/file")
{:ok, view, _} = live(conn, ~p"/open/storage")
path = Path.join(tmp_dir, "write_protected.livemd")
File.touch!(path)
@ -284,7 +284,7 @@ defmodule LivebookWeb.OpenLiveTest do
assert {:error, {:live_redirect, %{to: to}}} = live(conn, ~p"/open?path=#{directory_path}")
assert to == ~p"/open/file?path=#{directory_path}"
assert to == ~p"/open/storage?path=#{directory_path}"
{:ok, view, _} = live(conn, to)
assert render(view) =~ directory_path

View file

@ -1688,14 +1688,14 @@ defmodule LivebookWeb.SessionLiveTest do
describe "file management" do
@tag :tmp_dir
test "adding :attachment file entry from file",
test "adding :attachment file entry from storage",
%{conn: conn, session: session, tmp_dir: tmp_dir} do
Session.subscribe(session.id)
path = Path.join(tmp_dir, "image.jpg")
File.write!(path, "content")
{:ok, view, _} = live(conn, ~p"/sessions/#{session.id}/add-file/file")
{:ok, view, _} = live(conn, ~p"/sessions/#{session.id}/add-file/storage")
view
|> element(~s{form[phx-change="set_path"]})
@ -1721,11 +1721,11 @@ defmodule LivebookWeb.SessionLiveTest do
end
@tag :tmp_dir
test "adding :file file entry from file", %{conn: conn, session: session, tmp_dir: tmp_dir} do
test "adding :file file entry from storage", %{conn: conn, session: session, tmp_dir: tmp_dir} do
path = Path.join(tmp_dir, "image.jpg")
File.write!(path, "content")
{:ok, view, _} = live(conn, ~p"/sessions/#{session.id}/add-file/file")
{:ok, view, _} = live(conn, ~p"/sessions/#{session.id}/add-file/storage")
view
|> element(~s{form[phx-change="set_path"]})