Related pubs in form #910

allisonking · 2025-01-21T21:09:01Z

Issue(s) Resolved

Most of #791

High-level Explanation of PR

Adds related pub configuration to the external fill form.

Test Plan

Add a relationship field with any core schema type. I haven't tested them all yet, but I know that String, StringArray, Null, and Email all work. And I know that MemberSelect does NOT currently work.
Add this relationship field to a form via the form builder
You should be able to configure both the relationship and the value
Now go to the external fill page for this form. It should display your 'pub relation' fields

You should be able to link other existing pubs

And also configure the 'value' for the relationship
This should work for more complex fields like StringArrays too and include validation on the 'value' field
Save the form. Visiting the form again should load your values (you can visit again by visiting the url http://localhost:3000/c/{communitySlug}/public/forms/{formSlug}/fill?pubId={pubId})
You can also visit the pub page for this created pub and see your related pubs

Screenshots (if applicable)

Notes

Known issues:

MemberSelect doesn't work as related pub value

core/lib/server/pub.ts

allisonking · 2025-01-21T22:59:34Z

core/app/components/forms/FormElement.tsx

@@ -175,6 +176,18 @@ export const FormElement = ({
 		);
 	}

+	if (element.isRelation) {
+		const valueComponent = input;


I had a small hope this would Just Work, but it doesn't :) We want to render an "wrapper" component and an "inner" component

The 'wrapper' component is the 'Pub Relations' box, and the 'inner' component is the popover 'Role' in this screenshot. Without taking any of the changes in this PR into account, FormElement would return the contents of 'Role', which is what we want, just in the popover instead, so I stashed that as valueComponent here. Then render the 'wrapper' as RelatedPubsElement.

This doesn't actually work though because the value of the form is going to be an array now—something like {'croccroc:author': [ {value: 'admin', relatedPubId: 'xxx'} ]} so currently we end up rendering
.

The only way I can think of to get this to work is to render a separate component inside RelatedPubsElement based on the CoreSchemaType that is a lot like FormElement.tsx but doesn't make all the same assumptions, and might not use i.e. TextInputElement.tsx since that tries to get the field value directly when really we need to index into the array.

maybe I'm missing a simpler way to do this though??

allisonking · 2025-01-23T16:47:35Z

core/app/components/FormBuilder/FormBuilder.tsx

@@ -95,49 +96,12 @@ const elementPanelTitles: Record<PanelState["state"], string> = {
 	editingButton: "Edit Submission Button",
 };

-const PanelHeader = ({ state }: { state: PanelState["state"] }) => {


I moved all the side panel stuff to another component so I could reuse it in the form fill page. I think this would be better off using a drawer or maybe even the new side bar component from shadcn, but for now I reused what we had to keep things consistent

allisonking · 2025-01-23T16:52:21Z

core/app/components/forms/FormElement.tsx

@@ -1,48 +1,24 @@
-import { defaultComponent } from "schemas";


Refactored this to be a wrapper around PubFieldFormElement which now takes a lot of the inside of this function. This is because I needed to reuse the parts in PubFieldFormElement to render the inner value component:

allisonking · 2025-01-23T16:56:12Z

core/app/components/forms/PubFieldFormElement.tsx

@@ -0,0 +1,172 @@
+import { defaultComponent } from "schemas";


pretty much directly pulled from FormElement.tsx

allisonking · 2025-01-23T16:58:58Z

core/lib/server/pub.ts

+			(pv) => pv.relatedPubId
+		);
+
+		if (pubValuesWithRelations.length) {


this might result in an inconsistent return—should I update replacePubRelationsBySlug to also return something like what the .insertInto("pub_values") below does and then combine them?

i think that if this works then just leave it, i'm currently rewriting a bunch of this anyway!

allisonking · 2025-01-23T17:00:05Z

packages/ui/src/form.tsx

-			body = error[0]?.message;
+			const firstErrorIndex = error.findIndex((e) => !!e);
+			const firstError = error[firstErrorIndex];
+			body = firstError?.message ?? `Error with value at index ${firstErrorIndex}`;


this isn't a great error message, but I wasn't sure a better way to get something to show up when there is an error with a value in one of the array fields. This shows up as

when the 'value' field is empty

allisonking · 2025-02-03T15:04:51Z

core/app/components/FormBuilder/ElementPanel/InputComponentConfigurationForm.tsx

-		demoComponent: ({ element }) => (
-			<Input
-				placeholder={
-					element.schemaName === CoreSchemaType.String ? "For short text" : "For numbers"
-				}
-				type={element.schemaName === CoreSchemaType.String ? "text" : "number"}
-			/>
-		),
+		demoComponent: ({ element }) => {
+			const isNumber = element.schemaName === CoreSchemaType.Number;
+			return (
+				<Input
+					placeholder={isNumber ? "For numbers" : "For short text"}
+					type={isNumber ? "number" : "text"}
+				/>
+			);
+		},


email and url were showing up as number input components in the demo portion

allisonking · 2025-02-03T15:06:19Z

This is ready for review but MemberSelect doesn't work—if chosen, there'll be an error that I haven't put a lot of time into investigating yet, but I suspect it may be thorny! So I'm thinking I'll split that out into a separate chunk of work.

tefkah

Really really nice, great work! I left some suggestions about slightly nicer error displays for the values and using pubtitles in a bunch of places, if you incorporate those im happy to merge it!

tefkah · 2025-02-04T14:35:15Z

core/app/components/forms/AddRelatedPubsPanel.tsx

+				return (
+					<div className="flex items-center gap-2">
+						<span>{row.original.title || row.original.id}</span>
+					</div>


I think we should use the PubTitle here, rather than the id as fallback. Is somewhat clearer, and I don't think we want to show external users these ids

Suggested change

</div>

<span>{getPubTitle(row.original)}</span>

tefkah · 2025-02-04T14:35:25Z

core/app/components/forms/AddRelatedPubsPanel.tsx

+
+import type { GetPubsResult } from "~/lib/server";
+import { PanelHeader, SidePanel } from "~/app/components/SidePanel";
+import { DataTable } from "../DataTable/v2/DataTable";


see below

Suggested change

import { DataTable } from "../DataTable/v2/DataTable";

import { getPubTitle } from "~/lib/pubs";

tefkah · 2025-02-05T11:56:04Z

core/app/components/forms/elements/RelatedPubsElement.tsx

+	const configLabel = "label" in element.config ? element.config.label : undefined;
+	const label = configLabel || element.label || slug;
+
+	const { watch } = useFormContext();
+	const [isPopoverOpen, setPopoverIsOpen] = useState(false);
+	const value = watch(slug);
+	const showValue = value != null && value !== "" && !isPopoverOpen;
+
+	if (element.component === null) {
+		return null;


I think this way we have slightly better error reporting

Suggested change

const configLabel = "label" in element.config ? element.config.label : undefined;

const label = configLabel || element.label || slug;

const { watch } = useFormContext();

const [isPopoverOpen, setPopoverIsOpen] = useState(false);

const value = watch(slug);

const showValue = value != null && value !== "" && !isPopoverOpen;

if (element.component === null) {

return null;

}: PubFieldFormElementProps & { slug: RelatedPubValueSlug }) => {

const configLabel = "label" in element.config ? element.config.label : undefined;

const label = configLabel || element.label || slug;

const { watch, formState } = useFormContext<FormValue>();

const [isPopoverOpen, setPopoverIsOpen] = useState(false);

const value = watch(slug);

const showValue = value != null && value !== "" && !isPopoverOpen;

const [baseSlug, index] = slug.split(".");

const valueError = formState.errors[baseSlug]?.[parseInt(index)]?.value;

if (element.component === null) {

return null;

tefkah · 2025-02-05T11:56:40Z

core/app/components/forms/elements/RelatedPubsElement.tsx

+import { MultiBlock } from "ui/multiblock";
+import { Popover, PopoverContent, PopoverTrigger } from "ui/popover";
+
+import type { PubFieldFormElementProps } from "../PubFieldFormElement";
+import type { ElementProps } from "../types";
+import type { GetPubsResult } from "~/lib/server";
+import { AddRelatedPubsPanel } from "~/app/components/forms/AddRelatedPubsPanel";
+import { useContextEditorContext } from "../../ContextEditor/ContextEditorContext";
+import { useFormElementToggleContext } from "../FormElementToggleContext";
+import { PubFieldFormElement } from "../PubFieldFormElement";
+
+const RelatedPubBlock = ({
+	pub,
+	onRemove,
+	valueComponentProps,
+	slug,
+}: {
+	pub: Pick<GetPubsResult[number], "title" | "id">;
+	onRemove: () => void;
+	valueComponentProps: PubFieldFormElementProps;
+	slug: string;
+}) => {


necessities for the error reporting down below

Suggested change

import { MultiBlock } from "ui/multiblock";

import { Popover, PopoverContent, PopoverTrigger } from "ui/popover";

import type { PubFieldFormElementProps } from "../PubFieldFormElement";

import type { ElementProps } from "../types";

import type { GetPubsResult } from "~/lib/server";

import { AddRelatedPubsPanel } from "~/app/components/forms/AddRelatedPubsPanel";

import { useContextEditorContext } from "../../ContextEditor/ContextEditorContext";

import { useFormElementToggleContext } from "../FormElementToggleContext";

import { PubFieldFormElement } from "../PubFieldFormElement";

const RelatedPubBlock = ({

pub,

onRemove,

valueComponentProps,

slug,

}: {

pub: Pick<GetPubsResult[number], "title" | "id">;

onRemove: () => void;

valueComponentProps: PubFieldFormElementProps;

slug: string;

}) => {

import { Plus, Trash, TriangleAlert } from "ui/icon";

import { MultiBlock } from "ui/multiblock";

import { Popover, PopoverContent, PopoverTrigger } from "ui/popover";

import { cn } from "utils";

import type { PubFieldFormElementProps } from "../PubFieldFormElement";

import type { ElementProps } from "../types";

import type { GetPubsResult } from "~/lib/server";

import { AddRelatedPubsPanel } from "~/app/components/forms/AddRelatedPubsPanel";

import { getPubTitle } from "~/lib/pubs";

import { useContextEditorContext } from "../../ContextEditor/ContextEditorContext";

import { useFormElementToggleContext } from "../FormElementToggleContext";

import { PubFieldFormElement } from "../PubFieldFormElement";

type RelatedPubValueSlug = `${string}.${number}.value`;

const RelatedPubBlock = ({

pub,

onRemove,

valueComponentProps,

slug,

}: {

pub: Pick<GetPubsResult[number], "title" | "id" | "createdAt" | "pubType" | "values">;

onRemove: () => void;

valueComponentProps: PubFieldFormElementProps;

slug: RelatedPubValueSlug;

tefkah · 2025-02-05T11:56:59Z

core/app/components/forms/elements/RelatedPubsElement.tsx

+														remove(index);
+													};
+													const innerSlug = `${slug}.${index}.value`;
+													return (


final part for the error reporting above

Suggested change

return (

const innerSlug =

`${slug}.${index}.value` as const;

tefkah · 2025-02-05T12:00:08Z

core/app/components/forms/elements/RelatedPubsElement.tsx

+		<div className="flex items-center justify-between rounded border border-l-[12px] border-l-emerald-100 p-3">
+			<div className="flex flex-col items-start gap-1 text-sm">
+				<span className="font-semibold">{title || id}</span>
+				<ConfigureRelatedValue {...valueComponentProps} slug={slug} />


I think we should use the pubtitle here as well

Suggested change

<ConfigureRelatedValue {...valueComponentProps} slug={slug} />

<span className="font-semibold">{getPubTitle(pub)}</span>

tefkah · 2025-02-05T12:02:51Z

core/lib/server/pub.ts

+			(pv) => pv.relatedPubId
+		);
+
+		if (pubValuesWithRelations.length) {


i think that if this works then just leave it, i'm currently rewriting a bunch of this anyway!

Co-authored-by: Thomas F. K. Jorna <[email protected]>

allisonking · 2025-02-05T22:31:16Z

thanks for the great improvements @tefkah ! going to merge this now, but flagging that not all of the value inputs work yet, especially member select. but those should be fixed by #926 and #924

allisonking force-pushed the aking/791/related-pubs-in-form branch from 0648f6c to 3751834 Compare January 21, 2025 21:10

allisonking commented Jan 21, 2025

View reviewed changes

core/lib/server/pub.ts Show resolved Hide resolved

allisonking commented Jan 21, 2025

View reviewed changes

allisonking added 14 commits January 23, 2025 11:26

Add RelatedPubsElement

8de637b

Add side panel

ef1047f

Add pub selection

c5dc1a1

wip saving values

44f9d2e

Remove pubtype check from related pubs form element

4e32399

maybe normalize pub update?

c998757

Add related pubs when creating a pub

f76e2ab

Start adding value component

3402e49

Use useFieldArray to make array manipulation easier

5d7466c

Refactor so related pubs can use the same form element

6e32a06

Make updatePub work for related values

4a48630

Fix tests

3178f14

A few more fixes

46379df

Handle null related fields

1995b69

allisonking force-pushed the aking/791/related-pubs-in-form branch from 7a509bb to 1995b69 Compare January 23, 2025 16:44

allisonking commented Jan 23, 2025

View reviewed changes

allisonking added 7 commits January 23, 2025 12:15

Fix demo component for text input

7586f01

Add related pub e2e test

33c2bfd

Fix button type to stop inadvertent submitting

0b7067d

Merge branch 'main' into aking/791/related-pubs-in-form

7dcea5d

Try increasing timeout

ef7321f

Add wait for

bcefc7b

Merge branch 'main' into aking/791/related-pubs-in-form

226b602

allisonking commented Feb 3, 2025

View reviewed changes

allisonking marked this pull request as ready for review February 3, 2025 15:06

allisonking requested review from tefkah, kalilsn and 3mcd February 3, 2025 17:32

Fix date instantiation of related fields

5bbbcaf

allisonking mentioned this pull request Feb 4, 2025

Member select refactor #926

Merged

tefkah approved these changes Feb 5, 2025

View reviewed changes

allisonking and others added 3 commits February 5, 2025 16:23

Use pub title

94204a2

Co-authored-by: Thomas F. K. Jorna <[email protected]>

Add better error message

fcea612

Co-authored-by: Thomas F. K. Jorna <[email protected]>

Merge branch 'main' into aking/791/related-pubs-in-form

ad1d545

allisonking merged commit ecc17fa into main Feb 5, 2025
6 checks passed

allisonking deleted the aking/791/related-pubs-in-form branch February 5, 2025 22:31

allisonking mentioned this pull request Feb 6, 2025

Related pubs value components fixups #924

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Related pubs in form #910

Related pubs in form #910

allisonking commented Jan 21, 2025 •

edited

Loading

allisonking Jan 21, 2025

allisonking Jan 23, 2025

allisonking Jan 23, 2025

allisonking Jan 23, 2025

allisonking Jan 23, 2025

tefkah Feb 5, 2025

allisonking Jan 23, 2025

allisonking Feb 3, 2025

allisonking commented Feb 3, 2025

tefkah left a comment

tefkah Feb 4, 2025

tefkah Feb 4, 2025

tefkah Feb 5, 2025

tefkah Feb 5, 2025

tefkah Feb 5, 2025

tefkah Feb 5, 2025

tefkah Feb 5, 2025

allisonking commented Feb 5, 2025

		@@ -1,48 +1,24 @@
		import { defaultComponent } from "schemas";

		@@ -0,0 +1,172 @@
		import { defaultComponent } from "schemas";

	import { DataTable } from "../DataTable/v2/DataTable";
	import { getPubTitle } from "~/lib/pubs";

	<ConfigureRelatedValue {...valueComponentProps} slug={slug} />
	<span className="font-semibold">{getPubTitle(pub)}</span>

Related pubs in form #910

Related pubs in form #910

Conversation

allisonking commented Jan 21, 2025 • edited Loading

Issue(s) Resolved

High-level Explanation of PR

Test Plan

Screenshots (if applicable)

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonking commented Feb 3, 2025

tefkah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonking commented Feb 5, 2025

allisonking commented Jan 21, 2025 •

edited

Loading