Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for reloadDocument prop #286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add support for reloadDocument prop #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions apps/test-app/app/routes/submission.reload-document.tsx
View file
Open in desktop
@@ -0,0 +1,26 @@
import { DataFunctionArgs, redirect } from "@remix-run/server-runtime";
import { withZod } from "@remix-validated-form/with-zod";
import { ValidatedForm } from "remix-validated-form";
import { z } from "zod";
import { SubmitButton } from "~/components/SubmitButton";

const schema = z.object({});
const validator = withZod(schema);

export const action = async (args: DataFunctionArgs) => {
return redirect("/reload-document-test.html");
};

export default function SubmissionReloadDocument() {
return (
<>
<ValidatedForm validator={validator} method="post" reloadDocument>
<SubmitButton label="Submit reloadDocument form" />
</ValidatedForm>

<ValidatedForm validator={validator} method="post">
<SubmitButton label="Submit standard form" />
</ValidatedForm>
</>
);
}
16 changes: 16 additions & 0 deletions apps/test-app/cypress/integration/submission.ts
View file
Open in desktop
Expand Up @@ -256,4 +256,20 @@ describe("Submission", () => {

cy.findByText("Submit").should("exist");
});

describe("reloadDocument", () => {
it("should perform a full page submission when true", () => {
cy.visit("submission/reload-document").waitForJs();
cy.findByText("Submit reloadDocument form").click();
cy.findByText(
"Browser was properly redirected to a page outside of remix"
);
});

it("should perform an ajax submission when false", () => {
cy.visit("submission/reload-document").waitForJs();
cy.findByText("Submit standard form").click();
cy.findByText("404: Not Found");
});
});
});
8 changes: 8 additions & 0 deletions apps/test-app/public/reload-document-test.html
View file
Open in desktop
@@ -0,0 +1,8 @@
<html>
<title>Reload Document Worked</title>

<body>
<h1>Browser was properly redirected to a page outside of remix</h1>
</body>

</html>
22 changes: 14 additions & 8 deletions packages/remix-validated-form/src/ValidatedForm.tsx
View file
Open in desktop
Expand Up @@ -217,6 +217,7 @@ export function ValidatedForm<DataType>({
method,
replace,
id,
reloadDocument,
...rest
}: FormProps<DataType>) {
const formId = useFormId(id);
Expand Down Expand Up @@ -364,18 +365,23 @@ export function ValidatedForm<DataType>({
<Form
ref={mergeRefs([formRef, formRefProp])}
{...rest}
reloadDocument={reloadDocument}
id={id}
action={action}
method={method}
replace={replace}
onSubmit={(e) => {
e.preventDefault();
handleSubmit(
e,
e.currentTarget,
(e as unknown as HTMLSubmitEvent).nativeEvent
);
}}
onSubmit={
reloadDocument
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As-is, this is going to skip all validations, which I'm not sure is desirable. Shouldn't we validate, then submit the form via a full page reload? Not sure off the top of my head how to handle that situation when there's a submitter other than the form though.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a good point. I was going after this with the thought process that if you wanted a full page reload you would be okay with the server handling validation, like when JS isn’t running clientside. I can see how that’s not a super desirable solution though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just hit this "bug" while upgrading from Remix 1.8, to 1.19.0.
Seems like something changed in the framework (probably when it was rewritten on top of react-router), that my use case fail:

<ValidatedForm reloadDocument> on a regular route, posting to a resource route that generates and download a PDF.

Used patch-package to implement this PR approach, but would definitely love something that keeps client-side validation.

Anything I can do to help this PR move forward?

? undefined
: (e) => {
e.preventDefault();
handleSubmit(
e,
e.currentTarget,
(e as unknown as HTMLSubmitEvent).nativeEvent
);
}
}
onReset={(event) => {
onReset?.(event);
if (event.defaultPrevented) return;
Expand Down