Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
iamkyrylo opened this issue Sep 17, 2024 · 4 comments · Fixed by #7197
Assignees
Labels
Type: Bug Issue contains a defect related to a specific component.
Milestone

Comments

@iamkyrylo
Copy link
Contributor

iamkyrylo commented Sep 17, 2024

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

  1. Go to StackBlitz
  2. Open App.test.jsx file
  3. Open terminal and run npm run test script
  4. See the test pass successfully
  5. 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.

@iamkyrylo iamkyrylo added the Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible label Sep 17, 2024
@melloware melloware added Type: Bug Issue contains a defect related to a specific component. and removed Status: Needs Triage Issue will be reviewed by Core Team and a relevant label will be added as soon as possible labels Sep 17, 2024
@melloware melloware added this to the 10.8.4 milestone Sep 17, 2024
@melloware
Copy link
Member

melloware commented Sep 17, 2024

See original ticket: #3302

PR: #3708

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

@iamkyrylo
Copy link
Contributor Author

iamkyrylo commented Sep 17, 2024

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
Copy link
Member

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

@iamkyrylo
Copy link
Contributor Author

iamkyrylo commented Sep 17, 2024

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issue contains a defect related to a specific component.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants