From 2db22706fd72fcbb60e63873f8e90546a701cb2e Mon Sep 17 00:00:00 2001
From: Benjamin Leonard <hello@benleonard.co.uk>
Date: Wed, 6 Nov 2024 11:12:19 +0000
Subject: [PATCH 1/4] Add side modal heading

---
 app/forms/firewall-rules-common.tsx | 147 +++++++++++++++-------------
 app/ui/lib/SideModal.tsx            |   2 +
 2 files changed, 80 insertions(+), 69 deletions(-)

diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx
index b0935c893b..2ae4064090 100644
--- a/app/forms/firewall-rules-common.tsx
+++ b/app/forms/firewall-rules-common.tsx
@@ -36,8 +36,10 @@ import { useVpcSelector } from '~/hooks/use-params'
 import { Badge } from '~/ui/lib/Badge'
 import { toComboboxItems, type ComboboxItem } from '~/ui/lib/Combobox'
 import { FormDivider } from '~/ui/lib/Divider'
+import { FieldLabel } from '~/ui/lib/FieldLabel'
 import { Message } from '~/ui/lib/Message'
 import * as MiniTable from '~/ui/lib/MiniTable'
+import { SideModal } from '~/ui/lib/SideModal'
 import { TextInputHint } from '~/ui/lib/TextInput'
 import { KEYS } from '~/ui/util/keys'
 import { ALL_ISH } from '~/util/consts'
@@ -415,46 +417,50 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
 
       {/* Really this should be its own <form>, but you can't have a form inside a form,
           so we just stick the submit handler in a button onClick */}
-      <div className="flex flex-col gap-3">
-        <h3 className="mb-4 text-sans-2xl">Targets</h3>
-        <Message
-          variant="info"
-          content={
-            <>
+      <SideModal.Heading>Targets</SideModal.Heading>
+
+      <Message
+        variant="info"
+        content={
+          <>
+            <p>
               Targets determine the instances to which this rule applies. You can target
               instances directly by name, or specify a VPC, VPC subnet, IP, or IP subnet,
-              which will apply the rule to traffic going to all matching instances. Targets
-              are additive: the rule applies to instances matching{' '}
+              which will apply the rule to traffic going to all matching instances.
+            </p>
+            <p>
+              Targets are additive: the rule applies to instances matching{' '}
               <span className="underline">any</span> target.
-            </>
-          }
-        />
-        <DynamicTypeAndValueFields
-          sectionType="target"
-          control={targetForm.control}
-          valueType={targetType}
-          items={toComboboxItems(targetItems[targetType])}
-          // HACK: reset the whole subform, keeping type (because we just set
-          // it). most importantly, this resets isSubmitted so the form can go
-          // back to validating on submit instead of change
-          onTypeChange={() =>
-            targetForm.reset({ type: targetForm.getValues('type'), value: '' })
-          }
-          onInputChange={(value) => targetForm.setValue('value', value)}
-          onSubmitTextField={submitTarget}
-        />
-        <MiniTable.ClearAndAddButtons
-          addButtonCopy="Add target"
-          disableClear={!targetValue}
-          onClear={() => targetForm.reset()}
-          onSubmit={submitTarget}
-        />
-      </div>
+            </p>
+          </>
+        }
+      />
+
+      <DynamicTypeAndValueFields
+        sectionType="target"
+        control={targetForm.control}
+        valueType={targetType}
+        items={toComboboxItems(targetItems[targetType])}
+        // HACK: reset the whole subform, keeping type (because we just set
+        // it). most importantly, this resets isSubmitted so the form can go
+        // back to validating on submit instead of change
+        onTypeChange={() =>
+          targetForm.reset({ type: targetForm.getValues('type'), value: '' })
+        }
+        onInputChange={(value) => targetForm.setValue('value', value)}
+        onSubmitTextField={submitTarget}
+      />
+      <MiniTable.ClearAndAddButtons
+        addButtonCopy="Add target"
+        disableClear={!targetValue}
+        onClear={() => targetForm.reset()}
+        onSubmit={submitTarget}
+      />
       {!!targets.value.length && <TypeAndValueTable sectionType="target" items={targets} />}
 
       <FormDivider />
 
-      <h3 className="mb-4 text-sans-2xl">Filters</h3>
+      <SideModal.Heading>Filters</SideModal.Heading>
       <Message
         variant="info"
         content={
@@ -471,9 +477,9 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
         {/* We have to blow this up instead of using TextField to get better 
             text styling on the label */}
         <div className="mt-2">
-          <label id="portRange-label" htmlFor="portRange" className="text-sans-lg">
+          <FieldLabel id="portRange-label" htmlFor="portRange">
             Port filters
-          </label>
+          </FieldLabel>
           <TextInputHint id="portRange-help-text" className="mb-2">
             A single destination port (1234) or a range (1234&ndash;2345)
           </TextInputHint>
@@ -523,45 +529,48 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
       )}
 
       <fieldset className="space-y-0.5">
-        <legend className="mb-2 mt-4 text-sans-lg">Protocol filters</legend>
+        {/* todo: abstract this label and checkbox pattern */}
+        <FieldLabel id="portRange-label" htmlFor="portRange" className="mb-2">
+          Protocol filters
+        </FieldLabel>
+        {/* <legend className="mb-2 mt-4 text-sans-lg">Protocol filters</legend> */}
         <ProtocolField control={control} protocol="TCP" />
         <ProtocolField control={control} protocol="UDP" />
         <ProtocolField control={control} protocol="ICMP" />
       </fieldset>
 
-      <div className="flex flex-col gap-3">
-        <h3 className="mt-4 text-sans-lg">Host filters</h3>
-        <Message
-          variant="info"
-          content={
-            <>
-              Host filters match the &ldquo;other end&rdquo; of traffic from the
-              target&rsquo;s perspective: for an inbound rule, they match the source of
-              traffic. For an outbound rule, they match the destination.
-            </>
-          }
-        />
-        <DynamicTypeAndValueFields
-          sectionType="host"
-          control={hostForm.control}
-          valueType={hostType}
-          items={toComboboxItems(hostFilterItems[hostType])}
-          // HACK: reset the whole subform, keeping type (because we just set
-          // it). most importantly, this resets isSubmitted so the form can go
-          // back to validating on submit instead of change
-          onTypeChange={() =>
-            hostForm.reset({ type: hostForm.getValues('type'), value: '' })
-          }
-          onInputChange={(value) => hostForm.setValue('value', value)}
-          onSubmitTextField={submitHost}
-        />
-        <MiniTable.ClearAndAddButtons
-          addButtonCopy="Add host filter"
-          disableClear={!hostValue}
-          onClear={() => hostForm.reset()}
-          onSubmit={submitHost}
-        />
-      </div>
+      <FormDivider />
+
+      <SideModal.Heading>Host filters</SideModal.Heading>
+
+      <Message
+        variant="info"
+        content={
+          <>
+            Host filters match the &ldquo;other end&rdquo; of traffic from the
+            target&rsquo;s perspective: for an inbound rule, they match the source of
+            traffic. For an outbound rule, they match the destination.
+          </>
+        }
+      />
+      <DynamicTypeAndValueFields
+        sectionType="host"
+        control={hostForm.control}
+        valueType={hostType}
+        items={toComboboxItems(hostFilterItems[hostType])}
+        // HACK: reset the whole subform, keeping type (because we just set
+        // it). most importantly, this resets isSubmitted so the form can go
+        // back to validating on submit instead of change
+        onTypeChange={() => hostForm.reset({ type: hostForm.getValues('type'), value: '' })}
+        onInputChange={(value) => hostForm.setValue('value', value)}
+        onSubmitTextField={submitHost}
+      />
+      <MiniTable.ClearAndAddButtons
+        addButtonCopy="Add host filter"
+        disableClear={!hostValue}
+        onClear={() => hostForm.reset()}
+        onSubmit={submitHost}
+      />
       {!!hosts.value.length && <TypeAndValueTable sectionType="host" items={hosts} />}
 
       {error && (
diff --git a/app/ui/lib/SideModal.tsx b/app/ui/lib/SideModal.tsx
index 63b1d89262..62d478c5e7 100644
--- a/app/ui/lib/SideModal.tsx
+++ b/app/ui/lib/SideModal.tsx
@@ -170,6 +170,8 @@ function SideModalBody({ children }: { children?: ReactNode }) {
 
 SideModal.Body = SideModalBody
 
+SideModal.Heading = classed.div`text-sans-semi-xl`
+
 SideModal.Section = classed.div`p-8 space-y-6 border-secondary`
 
 SideModal.Footer = ({ children, error }: { children: ReactNode; error?: boolean }) => (

From 57eaad824ce44165220cfa26d9bce69078173f63 Mon Sep 17 00:00:00 2001
From: Benjamin Leonard <hello@benleonard.co.uk>
Date: Wed, 6 Nov 2024 11:17:12 +0000
Subject: [PATCH 2/4] Remove commented input legend

---
 app/forms/firewall-rules-common.tsx | 1 -
 1 file changed, 1 deletion(-)

diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx
index 2ae4064090..2881dcbef2 100644
--- a/app/forms/firewall-rules-common.tsx
+++ b/app/forms/firewall-rules-common.tsx
@@ -533,7 +533,6 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
         <FieldLabel id="portRange-label" htmlFor="portRange" className="mb-2">
           Protocol filters
         </FieldLabel>
-        {/* <legend className="mb-2 mt-4 text-sans-lg">Protocol filters</legend> */}
         <ProtocolField control={control} protocol="TCP" />
         <ProtocolField control={control} protocol="UDP" />
         <ProtocolField control={control} protocol="ICMP" />

From f28e4636250406a2ce35870fa9df5ff0f5602d8d Mon Sep 17 00:00:00 2001
From: David Crespo <david.crespo@oxidecomputer.com>
Date: Wed, 6 Nov 2024 09:59:57 -0600
Subject: [PATCH 3/4] very important mt-2

---
 app/forms/firewall-rules-common.tsx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx
index 2881dcbef2..119f7d461d 100644
--- a/app/forms/firewall-rules-common.tsx
+++ b/app/forms/firewall-rules-common.tsx
@@ -428,7 +428,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
               instances directly by name, or specify a VPC, VPC subnet, IP, or IP subnet,
               which will apply the rule to traffic going to all matching instances.
             </p>
-            <p>
+            <p className="mt-2">
               Targets are additive: the rule applies to instances matching{' '}
               <span className="underline">any</span> target.
             </p>
@@ -474,7 +474,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
       />
 
       <div className="flex flex-col gap-3">
-        {/* We have to blow this up instead of using TextField to get better 
+        {/* We have to blow this up instead of using TextField to get better
             text styling on the label */}
         <div className="mt-2">
           <FieldLabel id="portRange-label" htmlFor="portRange">

From 00f49806e62df9bc2dd736dc9a06f7f33706c5fa Mon Sep 17 00:00:00 2001
From: David Crespo <david.crespo@oxidecomputer.com>
Date: Wed, 6 Nov 2024 10:04:41 -0600
Subject: [PATCH 4/4] cut one word to cut a whole line out of the targets info
 box

---
 app/forms/firewall-rules-common.tsx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx
index 119f7d461d..da348ee214 100644
--- a/app/forms/firewall-rules-common.tsx
+++ b/app/forms/firewall-rules-common.tsx
@@ -425,8 +425,8 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) =
           <>
             <p>
               Targets determine the instances to which this rule applies. You can target
-              instances directly by name, or specify a VPC, VPC subnet, IP, or IP subnet,
-              which will apply the rule to traffic going to all matching instances.
+              instances directly or specify a VPC, VPC subnet, IP, or IP subnet, which will
+              apply the rule to traffic going to all matching instances.
             </p>
             <p className="mt-2">
               Targets are additive: the rule applies to instances matching{' '}