-
Notifications
You must be signed in to change notification settings - Fork 7
Add form switcher #1160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add form switcher #1160
Conversation
13ee081
to
8948d60
Compare
f3c9d0e
to
87cecc5
Compare
a6adb69
to
ff6774d
Compare
5719a72
to
28d6c2a
Compare
3252290
to
27d51b4
Compare
27d51b4
to
05289b7
Compare
1fcd173
to
607b6ab
Compare
42efc45
to
1d4431c
Compare
1d4431c
to
9e4205f
Compare
ca09ebf
to
f24140d
Compare
sorry, i have some suggested changes i wanted to push up to a separate branch, but accidentally pushed them here, hence the force push revert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than the bug with switching the view while creating the form it works well!
even though i know its temporary, i do feel like the behavior/look of the switcher could be slightly nicer. i put up this pr with some suggested changes: #1246
merge it in if you want, or not!
otherwise lgtm!
} | ||
|
||
const result = await handleFormToken({ | ||
params, | ||
searchParams, | ||
onExpired: ({ params, searchParams, result }) => { | ||
return; | ||
}, | ||
}); | ||
|
||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice that we can get rid of this logic, i totally forgot it was there! good that we can just rely on the magic link/invite functionality now rather than this weird edge case
"use client"; | ||
|
||
import type { Dispatch, PropsWithChildren, SetStateAction } from "react"; | ||
|
||
import { createContext, useContext, useMemo, useState } from "react"; | ||
|
||
import type { Form } from "~/lib/server/form"; | ||
|
||
export type FormSwitcherContext = { | ||
selectedForm?: Form; | ||
setSelectedForm: Dispatch<SetStateAction<Form | undefined>>; | ||
}; | ||
|
||
const FormSwitcherContext = createContext<FormSwitcherContext>({ | ||
setSelectedForm: () => {}, | ||
}); | ||
|
||
type Props = PropsWithChildren<{ | ||
defaultForm?: Form; | ||
}>; | ||
|
||
export const FormSwitcherProvider = ({ defaultForm, children }: Props) => { | ||
const [selectedForm, setSelectedForm] = useState(defaultForm); | ||
|
||
const value = useMemo( | ||
() => ({ selectedForm, setSelectedForm }), | ||
[selectedForm, setSelectedForm] | ||
); | ||
|
||
return <FormSwitcherContext.Provider value={value}>{children}</FormSwitcherContext.Provider>; | ||
}; | ||
|
||
export const useFormSwitcherContext = () => useContext(FormSwitcherContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem to be used anywhere, maybe we can remove it?
searchParams: Promise< | ||
Record<string, string> & { pubTypeId: PubTypesId; form?: string; pubId?: PubsId } | ||
>; | ||
}) { | ||
const searchParams = await props.searchParams; | ||
const params = await props.params; | ||
const { communitySlug } = params; | ||
|
||
if (!searchParams.pubId) { | ||
const sparams = new URLSearchParams(searchParams); | ||
sparams.set("pubId", crypto.randomUUID()); | ||
redirect(`/c/${communitySlug}/pubs/create?${sparams.toString()}`); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to get around the following issue (see vid):
- Go to create a Pub.
- A pubId is internally initialized using
crypto.randomUUID()
in bc the UploadElement needs to know about it - Switch form
- Content is reloaded! therefore
crypto.randomUUID()
is called again! - Create Pub
- Redirect fails, bc clientside pubid is different from server one
To solve this, I madet this page always have a UUID through the searchParam. This is not optimal, if you navigate back from the edit page to this page it will behave somewhat strangely. That could be solved by doing an additional check if a UUID is provided, namely to check whether that Pub already exists. If it does, try once more.
I'm not a big fan of this solution, as it forces an extra redirect.
Another solution is to properly propagate the changes in UUID on the server to the client. The problem with this is the following:
- Go to create a Pub.
- Upload a File.
- Switch the view
- Create Pub
- Upload file "PubId" no longer matches the pub it belongs to.
Maybe that's not a big deal and we should just do that, ill leave it to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug
Screen.Recording.2025-05-14.at.14.16.47.mov
@@ -188,7 +196,7 @@ export async function PubEditor(props: PubEditorProps) { | |||
// Create the pubId before inserting into the DB if one doesn't exist. | |||
// FileUpload needs the pubId when uploading the file before the pub exists | |||
const isUpdating = !!pub?.id; | |||
const pubId = pub?.id ?? (randomUUID() as PubsId); | |||
const pubId = pub?.id ?? props.searchParams.pubId ?? (randomUUID() as PubsId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the other place where the pubId searchParam is used, see above comment
9154242
to
f24140d
Compare
and again... i made this (#1247) a separate PR as it was messing with the tests. |
Issue(s) Resolved
Resolves #1037
Resolves #1036
High-level Explanation of PR
This PR adds the form switcher component to forms and the pub details page, and fully implements the new form permissions introduced in #1134.
Test Plan
Screenshots (if applicable)
Notes