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.

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

tests(remix-react): add failing test for default Form action #3211

Closed
wants to merge 4 commits into from
Closed

tests(remix-react): add failing test for default Form action #3211

wants to merge 4 commits into from

Conversation

ionut-botizan
Copy link

Issue: #3133

The default <Form /> action should match the current location. Currently, only the path is preserved, but the search parameters are lost.

E.g. For the /login?redirectTo=/profile location and a <Form method="post" /> element, the generated form node looks like
<form method="post" action="/login">.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented May 17, 2022

Hi @ionut-botizan,

Welcome, and thank you for contributing to Remix!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected].

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented May 17, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@machour machour added the bug Something isn't working label May 21, 2022
Copy link
Collaborator

@chaance chaance left a comment

Choose a reason for hiding this comment

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

A few changes needed before we can merge. If an action value of . is provided, it is correct to strip the search params when resolving the route path. We only need to preserve the location's search if no action is provided.

integration/bug-report-test.ts Outdated Show resolved Hide resolved
packages/remix-react/components.tsx Outdated Show resolved Hide resolved
Ionut Botizan added 4 commits June 11, 2022 22:19
Preserve the current location's search params when the default Form action is used.
Now that the fix has been applied, the passing tests have been moved to
`integration/form-test` and the template has been reverted.
@MichaelDeBoey MichaelDeBoey changed the title bug(remix-react): add failing test for default Form action tests(remix-react): add failing test for default Form action Jun 11, 2022
@maranomynet
Copy link

Please someone resolve and merge this PR.

Always having to do this feels so daft.

const { pathname, search } = useLocation();

return <Form action={pathname+search}

@ionut-botizan
Copy link
Author

Hey guys! Are there any other changes or additions that you would like me to provide? I think I did everything that was asked in the initial review...
I don't mean to bother you. I realize there are more important issues out there I was just thinking that maybe you missed the fact that I responded to the review. 🙂

@chaance
Copy link
Collaborator

chaance commented Aug 4, 2022

Hi @ionut-botizan, sorry for the radio silence on this one. We actually have duplicated efforts a bit on this bug. We are building Remix's form components and hooks into React Router, so @brophdawg11 and I have been working in tandem on that separately. I think I must have forgotten about your work and started my own over in #3697. Very sorry about that.

To clean things up, I'm going to close this one. But I added you as a co-committer for 783c435 (#3697) and made sure your handle is included in the contributor list. I definitely want to make sure you're credited with identifying the problem and proposing the initial fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CLA Signed renderer:react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants