From b9af7249441bcdb2d9d25201370e85198fa26894 Mon Sep 17 00:00:00 2001
From: Jamil <jamilbk@users.noreply.github.com>
Date: Tue, 23 Jul 2024 22:19:33 -0700
Subject: [PATCH] fix(ux): Improve resource creation form and traffic filters
 (#5983)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Adds `radio_button_group` to style radio buttons as big buttons
- Tweaks `inline_errors` so that `w-full` doesn't apply; this fixes the
input width from jumping when errors pop up
- Fixes #4979
- Fixes #5239


<img width="1616" alt="Screenshot 2024-07-23 at 5 45 12 PM"
src="https://github.com/user-attachments/assets/847c7a80-4cb6-4c4b-9095-1e7a08be479f">
---
 .../web/lib/web/components/core_components.ex |   6 +-
 .../web/lib/web/components/form_components.ex |  40 ++++-
 .../web/lib/web/live/resources/components.ex  | 159 +++++++++---------
 elixir/apps/web/lib/web/live/resources/new.ex |  76 +++++++--
 4 files changed, 175 insertions(+), 106 deletions(-)

diff --git a/elixir/apps/web/lib/web/components/core_components.ex b/elixir/apps/web/lib/web/components/core_components.ex
index 938eb2ff3..10d09f315 100644
--- a/elixir/apps/web/lib/web/components/core_components.ex
+++ b/elixir/apps/web/lib/web/components/core_components.ex
@@ -360,13 +360,13 @@ defmodule Web.CoreComponents do
     ~H"""
     <p
       class={[
-        "w-full flex gap-3 text-sm leading-6",
+        "flex items-center gap-2 text-sm leading-6",
         "text-rose-600",
-        (@inline && "ml-3") || "mt-3"
+        (@inline && "ml-2") || "mt-2 w-full"
       ]}
       {@rest}
     >
-      <.icon name="hero-exclamation-circle-mini" class="mt-0.5 h-5 w-5 flex-none" />
+      <.icon name="hero-exclamation-circle-mini" class="h-4 w-4 flex-none" />
       <%= render_slot(@inner_block) %>
     </p>
     """
diff --git a/elixir/apps/web/lib/web/components/form_components.ex b/elixir/apps/web/lib/web/components/form_components.ex
index 03568dec9..d7d911ceb 100644
--- a/elixir/apps/web/lib/web/components/form_components.ex
+++ b/elixir/apps/web/lib/web/components/form_components.ex
@@ -32,8 +32,9 @@ defmodule Web.FormComponents do
 
   attr :type, :string,
     default: "text",
-    values: ~w(checkbox color date datetime-local email file hidden month number password
-               range radio readonly search group_select select tel text textarea time url week)
+    values:
+      ~w(checkbox color date datetime-local email file hidden month number password
+               range radio radio_button_group readonly search group_select select tel text textarea time url week)
 
   attr :field, Phoenix.HTML.FormField,
     doc: "a form field struct retrieved from the form, for example: @form[:email]"
@@ -97,7 +98,8 @@ defmodule Web.FormComponents do
     |> input()
   end
 
-  def input(%{type: "radio"} = assigns) do
+  # radio with label
+  def input(%{type: "radio", label: _label} = assigns) do
     ~H"""
     <div>
       <label class="flex items-center gap-2 text-neutral-900">
@@ -120,6 +122,24 @@ defmodule Web.FormComponents do
     """
   end
 
+  # radio without label
+  def input(%{type: "radio_button_group"} = assigns) do
+    ~H"""
+    <input
+      type="radio"
+      id={@id}
+      name={@name}
+      value={@value}
+      checked={@checked}
+      class={[
+        "hidden peer",
+        @class
+      ]}
+      {@rest}
+    />
+    """
+  end
+
   def input(%{type: "checkbox"} = assigns) do
     assigns =
       assigns
@@ -172,7 +192,8 @@ defmodule Web.FormComponents do
         class={[
           "text-sm bg-neutral-50",
           "border border-neutral-300 text-neutral-900 rounded",
-          "block w-full p-2",
+          "block p-2",
+          !@inline_errors && "w-full",
           @errors != [] && "border-rose-400 focus:border-rose-400"
         ]}
         multiple={@multiple}
@@ -213,7 +234,8 @@ defmodule Web.FormComponents do
         class={[
           "text-sm bg-neutral-50",
           "border border-neutral-300 text-neutral-900 rounded",
-          "block w-full p-2.5",
+          "block p-2.5",
+          !@inline_errors && "w-full",
           @errors != [] && "border-rose-400 focus:border-rose-400"
         ]}
         multiple={@multiple}
@@ -237,10 +259,11 @@ defmodule Web.FormComponents do
         id={@id}
         name={@name}
         class={[
-          "block w-full rounded sm:text-sm sm:leading-6",
+          "block rounded sm:text-sm sm:leading-6",
           "bg-neutral-50",
           "border border-neutral-300 rounded",
           "min-h-[6rem]",
+          !@inline_errors && "w-full",
           @errors != [] && "border-rose-400 focus:border-rose-400",
           @class
         ]}
@@ -295,7 +318,7 @@ defmodule Web.FormComponents do
         "flex",
         "text-sm text-neutral-900 bg-neutral-50",
         "border border-neutral-300 rounded",
-        "w-full",
+        !@inline_errors && "w-full",
         "focus-within:outline-none focus-within:border-accent-600",
         "peer-disabled:bg-neutral-50 peer-disabled:text-neutral-500 peer-disabled:border-neutral-200 peer-disabled:shadow-none",
         @errors != [] && "border-rose-400 focus:border-rose-400"
@@ -341,7 +364,8 @@ defmodule Web.FormComponents do
         id={@id}
         value={Phoenix.HTML.Form.normalize_value(@type, @value)}
         class={[
-          "block w-full",
+          "block",
+          !@inline_errors && "w-full",
           "p-2.5 rounded",
           "bg-neutral-50 text-neutral-900 text-sm",
           "border border-neutral-300",
diff --git a/elixir/apps/web/lib/web/live/resources/components.ex b/elixir/apps/web/lib/web/live/resources/components.ex
index d3a47118e..c030d6ba3 100644
--- a/elixir/apps/web/lib/web/live/resources/components.ex
+++ b/elixir/apps/web/lib/web/live/resources/components.ex
@@ -81,86 +81,89 @@ defmodule Web.Resources.Components do
         protocols and ports are accessible.
       </p>
 
-      <div class={@traffic_filters_enabled? == false && "opacity-50"}>
-        <div>
+      <div class={[
+        @traffic_filters_enabled? == false && "opacity-50",
+        "mt-4"
+      ]}>
+        <div class="flex items-top h-20">
+          <.input type="hidden" name={"#{@form.name}[tcp][protocol]"} value="tcp" />
+          <div class="mt-2.5 w-24">
+            <.input
+              title="Restrict traffic to TCP traffic"
+              type="checkbox"
+              field={@forms_by_protocol[:tcp]}
+              name={"#{@form.name}[tcp][enabled]"}
+              checked={Map.has_key?(@forms_by_protocol, :tcp)}
+              value="true"
+              disabled={!@traffic_filters_enabled?}
+              label="TCP"
+            />
+          </div>
+
+          <div class="flex-none">
+            <% ports = (@forms_by_protocol[:tcp] || %{ports: %{value: []}})[:ports] %>
+            <.input
+              type="text"
+              inline_errors={true}
+              field={ports}
+              name={"#{@form.name}[tcp][ports]"}
+              value={Enum.any?(ports.value) && pretty_print_ports(ports.value)}
+              disabled={!@traffic_filters_enabled? || !Map.has_key?(@forms_by_protocol, :tcp)}
+              placeholder="E.g. 80, 443, 8080-8090"
+              class="w-96"
+            />
+            <p class="mt-2 text-xs text-neutral-500">
+              List of comma-separated port range(s), Matches all ports if empty.
+            </p>
+          </div>
+        </div>
+
+        <div class="flex items-top h-20">
+          <.input type="hidden" name={"#{@form.name}[udp][protocol]"} value="udp" />
+          <div class="mt-2.5 w-24">
+            <.input
+              type="checkbox"
+              field={@forms_by_protocol[:udp]}
+              name={"#{@form.name}[udp][enabled]"}
+              checked={Map.has_key?(@forms_by_protocol, :udp)}
+              value="true"
+              disabled={!@traffic_filters_enabled?}
+              label="UDP"
+            />
+          </div>
+
+          <div class="flex-none">
+            <% ports = (@forms_by_protocol[:udp] || %{ports: %{value: []}})[:ports] %>
+            <.input
+              type="text"
+              inline_errors={true}
+              field={ports}
+              name={"#{@form.name}[udp][ports]"}
+              value={Enum.any?(ports.value) && pretty_print_ports(ports.value)}
+              disabled={!@traffic_filters_enabled? || !Map.has_key?(@forms_by_protocol, :udp)}
+              placeholder="E.g. 53, 60000-61000"
+              class="w-96"
+            />
+            <p class="mt-2 text-xs text-neutral-500">
+              List of comma-separated port range(s), Matches all ports if empty.
+            </p>
+          </div>
+        </div>
+
+        <div class="flex items-top h-20">
           <.input type="hidden" name={"#{@form.name}[icmp][protocol]"} value="icmp" />
 
-          <div class="items-center flex flex-row h-12">
-            <div class="flex-none w-32">
-              <.input
-                title="Allow ICMP traffic"
-                type="checkbox"
-                field={@forms_by_protocol[:icmp]}
-                name={"#{@form.name}[icmp][enabled]"}
-                checked={Map.has_key?(@forms_by_protocol, :icmp)}
-                value="true"
-                disabled={!@traffic_filters_enabled?}
-                label="ICMP"
-              />
-            </div>
-          </div>
-        </div>
-
-        <div>
-          <.input type="hidden" name={"#{@form.name}[tcp][protocol]"} value="tcp" />
-
-          <div class="items-center flex flex-row h-12">
-            <div class="flex-none w-32">
-              <.input
-                title="Restrict traffic to TCP traffic"
-                type="checkbox"
-                field={@forms_by_protocol[:tcp]}
-                name={"#{@form.name}[tcp][enabled]"}
-                checked={Map.has_key?(@forms_by_protocol, :tcp)}
-                value="true"
-                disabled={!@traffic_filters_enabled?}
-                label="TCP"
-              />
-            </div>
-
-            <div class="flex-grow">
-              <% ports = (@forms_by_protocol[:tcp] || %{ports: %{value: []}})[:ports] %>
-              <.input
-                type="text"
-                inline_errors={true}
-                field={ports}
-                name={"#{@form.name}[tcp][ports]"}
-                value={Enum.any?(ports.value) && pretty_print_ports(ports.value)}
-                disabled={!@traffic_filters_enabled? || !Map.has_key?(@forms_by_protocol, :tcp)}
-                placeholder="Comma-separated port range(s), eg. 433, 80, 90-99. Matches all ports if empty."
-              />
-            </div>
-          </div>
-        </div>
-
-        <div>
-          <.input type="hidden" name={"#{@form.name}[udp][protocol]"} value="udp" />
-
-          <div class="items-center flex flex-row h-12">
-            <div class="flex-none w-32">
-              <.input
-                type="checkbox"
-                field={@forms_by_protocol[:udp]}
-                name={"#{@form.name}[udp][enabled]"}
-                checked={Map.has_key?(@forms_by_protocol, :udp)}
-                value="true"
-                disabled={!@traffic_filters_enabled?}
-                label="UDP"
-              />
-            </div>
-
-            <div class="flex-grow">
-              <% ports = (@forms_by_protocol[:udp] || %{ports: %{value: []}})[:ports] %>
-              <.input
-                type="text"
-                inline_errors={true}
-                field={ports}
-                name={"#{@form.name}[udp][ports]"}
-                value={Enum.any?(ports.value) && pretty_print_ports(ports.value)}
-                disabled={!@traffic_filters_enabled? || !Map.has_key?(@forms_by_protocol, :udp)}
-                placeholder="Comma-separated port range(s), eg. 433, 80, 90-99. Matches all ports if empty."
-              />
-            </div>
+          <div class="mt-2.5 w-24">
+            <.input
+              title="Allow ICMP traffic"
+              type="checkbox"
+              field={@forms_by_protocol[:icmp]}
+              name={"#{@form.name}[icmp][enabled]"}
+              checked={Map.has_key?(@forms_by_protocol, :icmp)}
+              value="true"
+              disabled={!@traffic_filters_enabled?}
+              label="ICMP"
+            />
           </div>
         </div>
       </div>
diff --git a/elixir/apps/web/lib/web/live/resources/new.ex b/elixir/apps/web/lib/web/live/resources/new.ex
index 838a854bc..0d7dc9eac 100644
--- a/elixir/apps/web/lib/web/live/resources/new.ex
+++ b/elixir/apps/web/lib/web/live/resources/new.ex
@@ -37,44 +37,86 @@ defmodule Web.Resources.New do
           <h2 class="mb-4 text-xl text-neutral-900">Resource details</h2>
           <.form for={@form} class="space-y-4 lg:space-y-6" phx-submit="submit" phx-change="change">
             <div>
-              <label for="resource_type" class="block mb-2 text-sm text-neutral-900">
+              <p class="mb-2 text-sm text-neutral-900">
                 Type
-              </label>
-              <div class="flex text-sm leading-6 text-neutral-600">
-                <div class="flex items-center me-4">
+              </p>
+              <ul class="grid w-full gap-6 md:grid-cols-3">
+                <li>
                   <.input
                     id="resource-type--dns"
-                    type="radio"
+                    type="radio_button_group"
                     field={@form[:type]}
                     value="dns"
-                    label="DNS"
                     checked={@form[:type].value == :dns}
                     required
                   />
-                </div>
-                <div class="flex items-center me-4">
+                  <label for="resource-type--dns" class={~w[
+                    inline-flex items-center justify-between w-full
+                    p-5 text-gray-500 bg-white border border-gray-200
+                    rounded cursor-pointer peer-checked:border-accent-500
+                    peer-checked:text-accent-500 hover:text-gray-600 hover:bg-gray-100
+                  ]}>
+                    <div class="block">
+                      <div class="w-full font-semibold mb-3">
+                        <.icon name="hero-globe-alt" class="w-5 h-5 mr-1" /> DNS
+                      </div>
+                      <div class="w-full text-sm">
+                        Manage access to an application or service by DNS address.
+                      </div>
+                    </div>
+                  </label>
+                </li>
+                <li>
                   <.input
                     id="resource-type--ip"
-                    type="radio"
+                    type="radio_button_group"
                     field={@form[:type]}
                     value="ip"
-                    label="IP"
                     checked={@form[:type].value == :ip}
                     required
                   />
-                </div>
-                <div class="flex items-center me-4">
+                  <label for="resource-type--ip" class={~w[
+                    inline-flex items-center justify-between w-full
+                    p-5 text-gray-500 bg-white border border-gray-200
+                    rounded cursor-pointer peer-checked:border-accent-600
+                    peer-checked:text-accent-500 hover:text-gray-600 hover:bg-gray-100
+                  ]}>
+                    <div class="block">
+                      <div class="w-full font-semibold mb-3">
+                        <.icon name="hero-server" class="w-5 h-5 mr-1" /> IP
+                      </div>
+                      <div class="w-full text-sm">
+                        Manage access to a specific host by IP address.
+                      </div>
+                    </div>
+                  </label>
+                </li>
+                <li>
                   <.input
                     id="resource-type--cidr"
-                    type="radio"
+                    type="radio_button_group"
                     field={@form[:type]}
                     value="cidr"
-                    label="CIDR"
                     checked={@form[:type].value == :cidr}
                     required
                   />
-                </div>
-              </div>
+                  <label for="resource-type--cidr" class={~w[
+                    inline-flex items-center justify-between w-full
+                    p-5 text-gray-500 bg-white border border-gray-200
+                    rounded cursor-pointer peer-checked:border-accent-500
+                    peer-checked:text-accent-500 hover:text-gray-600 hover:bg-gray-100
+                  ]}>
+                    <div class="block">
+                      <div class="w-full font-semibold mb-3">
+                        <.icon name="hero-server-stack" class="w-5 h-5 mr-1" /> CIDR
+                      </div>
+                      <div class="w-full text-sm">
+                        Manage access to a network, VPC or subnet by CIDR address.
+                      </div>
+                    </div>
+                  </label>
+                </li>
+              </ul>
             </div>
 
             <div>
@@ -87,7 +129,7 @@ defmodule Web.Resources.New do
                     @form[:type].value == :dns -> "gitlab.company.com"
                     @form[:type].value == :cidr -> "10.0.0.0/24"
                     @form[:type].value == :ip -> "10.3.2.1"
-                    true -> "Please select a Type from the options first"
+                    true -> "First select a type above"
                   end
                 }
                 class={is_nil(@form[:type].value) && "cursor-not-allowed"}