blitz-js / legacy-framework

MIT License
2 stars 2 forks source link

react-hook-form button is not disabled for long enough #7

Open xdivby0 opened 2 years ago

xdivby0 commented 2 years ago

What do you want and why?

I am using react-hook-form and the new.tsx pages that get generated always have this part: <button type="submit" disabled={ctx.formState.isSubmitting}>

This means that after the form has been submitted successfully and isSubmitting is false again, we can double submit the form before the route changes (especially on slower connections) and create a second new entity by accident.

I'd like to do something like disabled={ctx.formState.isSubmitSuccessful || ctx.formState.isSubmitting} to prevent this accidental double submit.

Possible implementation(s)

Adding ctx.formState.isSubmitSuccessful should be possible pretty easy as documented in react-hook-form formState documentation.

beerose commented 2 years ago

Hi @xdivby0!

While I got the problem here, I don't think we can change it by providing disabled={ctx.formState.isSubmitSuccessful || ctx.formState.isSubmitting} by default. My reasoning is that sometimes, you do want the button enabled right after the click. Let's say you have a voting component and a button to add votes (something like Medium does). In that case you need to make sure users can click as much as they want.

Having said that, I think that modifying the form to fit particular use cases is something that should be done by the end users — after creating a new Blitz app, so that everyone gets the very basic thing, and then they can modify it depending on their needs.

Let me know what you think.

xdivby0 commented 2 years ago

@beerose I think this is a valid point for some use cases. Correct me if I'm wrong, but I see three reasons that speak against that:

  1. using the new.tsx for something like votes that are not inherently "new" objects should be the minor case in which modifications like removing the isSubmitSuccessful would be the thing people want, right?
  2. If I'm wrong in 1., then we need to remove the auto-redirect-after-form-submit by default too because of the same reason (the voting example would not work that way either).
  3. Seeing and removing the stuff you don't need (in this case the isSubmitSuccessful) is always easier than finding out what you need to add.

Does that make sense?

beerose commented 2 years ago

Hmm that makes sense, but what about the form in edit mode? Right now the forms are used for both creating and editing, while with edit users should be able to edit multiple times.

I don't have very strong opinions here, but want to make sure we won't worsen the experience for anyone.

xdivby0 commented 2 years ago

If I'm not wrong, currently edit forms as well as new forms both redirect after submit and I think this is the default people want (set / create something once). If you think more people want the edit to be used multiple times, you'd have to remove the redirect.

You are totally right with that new.tsx and edit.tsx share the same form component - and I think it should stay that way. If (as stated in the paragraph above) the behavior should be different for the edit and new version, maybe add a prop multiSubmit which can control whether the form is able to multiple times?

For me, there are 2 good choices now (which need further discussion or opinions):

Leaving it in this inconsistent state is weird. It's inconsistent that the redirect then would only happen on new.tsx, but not edit.tsx, although both of them do allow a re-submit / multisubmit (concerning the button).