seasonedcc / remix-forms

The full-stack form library for Remix and React Router
https://remix-forms.seasoned.cc
MIT License
493 stars 25 forks source link

Customize submit button pending state condition #172

Closed atoi closed 1 year ago

atoi commented 1 year ago

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 commented 1 year ago

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. #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 commented 1 year ago

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.

danielweinmann commented 1 year ago

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 commented 1 year ago

Released on v1.6.4