refinedev / refine

A React Framework for building internal tools, admin panels, dashboards & B2B apps with unmatched flexibility.
https://refine.dev
MIT License
28.56k stars 2.23k forks source link

[BUG] `onFinish` from `useModalForm` doesn't have same behavior as `onFinish` from `formProps` #6181

Closed tabrezm closed 3 months ago

tabrezm commented 3 months ago

Describe the bug

I ran into something I believe is a doc issue (or possibly a bug) with useModalForm from @refinedev/antd.

I implemented a form where I need to do a file upload upon submitting the form, so I provided my own method to onFinish in the Form component. Based on the doc, I thought I could call onFinish and then close. But I ran into the following behaviors:

  1. The user sees the dialog about unsaved changes (which seems like an issue on its own).
  2. If they click Cancel, they get correctly returned to the dialog.
  3. If they click OK (i.e., OK to discard changes), it ends up updating the resource anyway, which I can confirm from PUT/PATCH calls being made by the browser.

I realized after much trial and error that the right method to call is not onFinish from useModalForm, but onFinish from the formProps. Furthermore, I don't need to call close since onFinish seems to close the dialog already. So a couple questions:

  1. Is the sample in the doc correct?
    
    import { useModalForm } from "@refinedev/antd";
    import { Form, Input, Modal } from "antd";

const { close, modalProps, formProps, onFinish } = useModalForm();

const onFinishHandler = (values) => { // do some work onFinish(values); close(); };

// ---

return ( <Modal {...modalProps}> <Form {...formProps} onFinish={onFinishHandler} layout="vertical">

); ``` This repros the 3 behaviors I mentioned above. It seems like even if `onFinish` from `useModalForm` is the right method to call, the close call would be unnecessary. 2. Why is `onFinish` from `useModalForm` not the same as the one from `formProps`? Based on the [doc](https://refine.dev/docs/ui-integrations/ant-design/hooks/use-modal-form/#close), it says the only difference is passing a value is mandatory, but in my experience they handle submission and closing differently. ### Steps To Reproduce Use the sample for `close()` from the [doc](https://refine.dev/docs/ui-integrations/ant-design/hooks/use-modal-form/#close). ### Expected behavior Form closes without warning about unsaved changes, and the documentation correctly includes (or excludes) the use of `close()` with `onFinish()`. ### Packages @refinedev/antd ### Additional Context _No response_
aliemir commented 3 months ago

Hey @tabrezm sorry for the issue! Looks like there should be a small update to the docs and also the distinction between the two onFinish methods should be mentioned.

formProps.onFinish uses the onFinish internally and also includes two extra conditions, depending on the autoSubmitClose and autoResetForm props, it will call close() and form.resetFields(). One thing that is broken in the code sample is that onFinish is an async function and needs to be awaited before moving on to the close() call.

formProps.onFinish awaits the onFinish call and then continues with the close() but in the code sample await is missing and causes the issue with the unsaved changes notifier.

Let's keep this issue open and have the necessary changes to the docs to clear out misunderstandings. Let me know if the issue is resolved by adding a proper await to the onFinish call when you're using the onFinish from the useModalForm 🙏

tabrezm commented 3 months ago

Yes, I believe that combination worked. Thanks for the speedy reply!

Is there a better/best practice to follow on using one approach over the other? Only needing to pass the formProps is more convenient than passing around onFinish and close, and it seems like it's the higher-level encapsulation.