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

Customize submit button pending state condition #172

Closed
atoi opened this issue Mar 24, 2023 · 4 comments
Closed

Customize submit button pending state condition #172

atoi opened this issue Mar 24, 2023 · 4 comments

Comments

@atoi
Copy link

atoi commented Mar 24, 2023

TLDR: It would be nice to be able to customize the condition that toggles submit button "pending" sate.

Currently remix-forms uses transition.state === 'submitting' check to make the decision, but in some cases this is not quite the right way to go.

In my particular case I have the form+fetcher inside modal dialog and I want to close the dialog when submit is completed.

These are the state changes the fetcher goes through when sends POST request:

  1. { state: 'idle', type: 'init' } -- Just rendered and did nothing yet
  2. { state: 'submitting', type: 'actionSubmission' } -- Request is in progress
  3. { state: 'loading', type: 'actionReload' } -- Routes loaders revalidation is in progress
  4. { state: 'idle', type: 'done' } -- Data is both submitted and reloaded

To close the dialog at the right time I check for fetcher.type === 'done' inside onTransition.
In this case the form spends some time in loading state, so the button goes from disabled back to enabled, changes its label from pending back to regular one, and spends some time in that state before dialog is actually closed.

I find this behavior quite annoying, so I tried another option:
Check for fetcher.type === 'actionReload'. This works -- the dialog is closed at the "right" time, without letting the button to go back to "enabled". But it's better to be careful about the dialog and form implementation details. Because when the form is unmounted the fetcher gets destroyed and cancels revalidation requests. Also, there are cases when it actually may be necessary to wait for loaders revalidation.

I'm not going to claim that transition.state === 'submitting' || transition.state === 'loading' is the more correct condition, because there could be different scenarios. But it seems that having an extra prop, say getIsPending?: (state: 'idle' | 'submitting' | 'loading') => boolean would be really helpful.

@danielweinmann
Copy link
Contributor

Hey, @atoi! Thanks for the detailed issue 💪🏽 a few considerations:

  1. It would be good to change the default condition to transition.state !== 'idle' instead of the current transition.state === 'submitting'. Would you like to submit a PR for that?

  2. Even without the change in the default, you can already customize the behavior on a form-by-form basis. You just need to pass the disabled prop to Button explicitly. Remix Forms always gives precedence to explicit props over our magically generated ones.

  3. A hacky but generic solution would be to customize your buttonComponent to ignore the disabled prop it receives from Remix Forms and set it manually based on a useTransition or useFetchers state.

  4. For an elegant and generic solution, I don't think we need a prop like getIsPending. Add renderForm prop to Form #122, when implemented, will give us the ability to customize the rendering of Forms without children, including the logic for rendering buttons.

How does that sound?

@atoi
Copy link
Author

atoi commented Mar 27, 2023

Hi, @danielweinmann

  1. I'm not sure if changing the default condition is the right way to go. I mean, both state !== 'idle' and state === 'loading' have their usage in different scenarios. That's why I suggested an extra prop.
  2. As for setting disabled manually -- that's what I ended up doing. Due to using framer-motion I rendered the form "manually" anyways.
  3. Hmm, yes. Actually this one I kind of like. Will try to resort to this one when I get tired of setting up "disabled" manually. Or when I will need the "proper" behavior without rendering form manually.
  4. You're right. This one probably is a winner, so let's not pollute props api with an extra one =)

Thank you for the detailed answer. I think we can consider this one resolved.

@atoi atoi closed this as completed Mar 27, 2023
@danielweinmann
Copy link
Contributor

I'm reopening this because I think we should change the default condition to transition.state !== 'idle' instead of the current transition.state === 'submitting'. It's causing a flash of enabling the submit button on most cases before redirecting.

I'll leave this issue open until we implement it.

@danielweinmann
Copy link
Contributor

Released on v1.6.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants