Opening overlay with timeout causes unexpected behavior during testing components with MultiSelect

iamkyrylo commented 1 month ago

Describe the bug

The issue is caused by setTimeout in useEffect to show or hide the overlay panel with a delay (lines of code). This forces you to wait for the timeout in tests after mounting components with MultiSelect. Without this wait, the second option won’t be selected, and the test will fail🤷‍♂️

I’m not sure why this delay was added, but after removing it and testing locally, everything worked fine.

Reproducer

https://stackblitz.com/edit/dxmu22?file=src%2FApp.test.jsx

System Information

"primereact": "latest"
"react": "18.3.1"
"react-dom": "18.3.1"

Steps to reproduce the behavior

Go to StackBlitz
Open App.test.jsx file
Open terminal and run npm run test script
See the test pass successfully
Comment out the line with wait, and you’ll see the second option won’t be selected, leading to the following error:
```
Expected value: New York, Rome
Received: New York
```

Expected behavior

The MultiSelect component should allow selecting multiple options without needing to wait for a timeout in tests. All options should be selectable as expected after opening the dropdown, and the component should behave consistently.

melloware commented 1 month ago

See original ticket: https://github.com/primefaces/primereact/issues/3302

PR: https://github.com/primefaces/primereact/pull/3708

Its only when using overlayVisible={true} the user wanted the component displayed when the page was loaded.

iamkyrylo commented 1 month ago

Its only when using overlayVisible={true} the user wanted the component displayed when the page was loaded.

Actually, this doesn’t only occur when the overlayVisible prop is passed, because the condition is defined inside the timeout. A better solution would be to wrap setTimeout in the condition and call it only after if (props.overlayVisible) and ensure that overlayVisible is a boolean for both: show and hide cases, like:

React.useEffect(() => {
    if (props.overlayVisible === true) {
        setTimeout(() => show(), 100);
    } else if (props.overlayVisible === false && overlayRef.current.isVisible) {
        setTimeout(() => hide(), 100);
    }
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [props.overlayVisible]);

(I'm also assuming that the timeout is not needed to hide the overlay🤔)

I can add this to my PR, but I’d like some time to think of a more effective approach without setTimeout

melloware commented 1 month ago

no problem let me know when your PR is ready and I will review it!

iamkyrylo commented 1 month ago

@melloware I found the solution! The issue was simpler than I initially thought😅

The problem was that onEnter callback wasn’t being called during the initial mount of the overlay when overlayVisible={true} is passed. But, CSSTransition has a fix for this - the appear property. All we need to do to make it work without timeouts is pass appear: true to the CSSTransition through the transitionProps, which will trigger onEnter callback during the initial mount and align the panel properly. Check out my PR with the updates!

via SpongeBob SquarePants on GIPHY

primefaces / primereact