fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Feature]: It would be great if the Dialog component supported a parameter like "onClose".

Open phuctvt opened this issue 1 year ago • 3 comments

Library

React Components / v9 (@fluentui/react-components)

Describe the feature that you would like added

Hello Fluent UI team,

I'm using the Fluent UI and I love it. Recently there was a situation in a project I'm working on, it's something like this sample: https://stackblitz.com/edit/urlkjp.

In the demo sample, if you select a product, wait ~2 seconds for it to finish loading the product descriptions, then click "Close", the dialog starts closing as normal, but at the moment it is fading out, we can see it has a flickering. Here is my screen recording of the slowed down what happened to easier show what I mean:

https://github.com/microsoft/fluentui/assets/20752858/2a2937fb-e683-4c60-80b5-620b6ef1d1fd

The reason is in the file ProductListDialog.tsx, at line 41, the cleanUp() is called at the moment the dialog is fading out, it means the dialog is not closed completely yet, until the CSS animation finishes.

I think it would be better if the Dialog component supported a parameter like onClose: () => void. The onClose should be called when the dialog is completely closed, including the CSS animation. So I can assign my cleanUp to this param to avoid the flickering.


One workaround I found is, I can create a new below component and use it instead of the original DialogSurface, then I can assign cleanUp to onDialogClosed:

import React, { useEffect } from "react";
import { DialogSurfaceProps, DialogSurface as RealDialogSurface } from "@fluentui/react-components";

export function DialogSurface(
    props: DialogSurfaceProps & {
        /**
         * Callback that is called when the dialog is completely closed, including the animation.
         */
        onDialogClosed?: () => void;
    }
) {
    useEffect(() => {
        return () => {
            // This callback is called when the RealDialogSurface element is removed from DOM.
            props.onDialogClosed?.();
        };
    }, []);

    return <RealDialogSurface {...props}>{props.children}</RealDialogSurface>;
}

Here is a demo for it: https://stackblitz.com/edit/urlkjp-nlnoaz.

Have you discussed this feature with our team

No response

Additional context

No response

Validations

  • [X] Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

Priority

None

phuctvt avatar Apr 21 '24 08:04 phuctvt

Waiting for motion events support.

miroslavstastny avatar Apr 22 '24 14:04 miroslavstastny

Create a separate component for the product listing and description and manage the load state from there instead.

Each time the dialog is opened, the new component should be re mounted

jacksonv1lle avatar Apr 26 '24 21:04 jacksonv1lle

Hi @jacksonv1lle, it will work.

However, I think it's quite inconvenient for fairly common use cases. I usually see components that have names like this format SomethingDialog that wrap some business logic. For example, in cases of ProductListDialog, ConfirmationDialog, or LoginDialog, we have to create two components for each case.

phuctvt avatar Apr 30 '24 13:04 phuctvt

Blocked by #30700.

layershifter avatar May 16 '24 09:05 layershifter

Hey @phuctvt, #31132 has been merged.

You will be able to do <Dialog surfaceMotion={{ onMotionFinish: () => {} } /> with a next release 😉

layershifter avatar Jul 17 '24 12:07 layershifter

Super cool! Thank you and the team.

phuctvt avatar Jul 17 '24 13:07 phuctvt