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

Fix #3302: Multiselect add overlayVisible property #3708

Merged
merged 3 commits into from
Nov 27, 2022

Conversation

melloware
Copy link
Member

Defect Fixes

Fix #3302: Multiselect add overlayVisible property

PrimeNG has this property

@melloware melloware added the Core Team Issue or pull request has been *opened* by a member of Core Team label Nov 25, 2022
@melloware melloware merged commit 39f4a03 into primefaces:master Nov 27, 2022
@melloware melloware deleted the PR3302 branch November 27, 2022 14:37
@kalinkrustev
Copy link
Contributor

kalinkrustev commented Dec 5, 2022

I think this is not a very good approach. The setTimeout here:

        React.useEffect(() => {
            setTimeout(() => {
                props.overlayVisible ? show() : hide();
            }, 100);
        }, [props.overlayVisible])

can potentially throw Can't perform a React state update on an unmounted component., as during these 100 milliseconds, the component could be unmounted. I am observing this in a Jest test, but it can potentially happen in the browser too.
While there is a solution to cancel the timeout (which will make tests more complicated), I think a better approach is to decide first about what is the purpose of props.overlayVisible. I think it should be one of these:

  • it only controls the initial visibility of the overlay, in which case the setTimeout may not be needed
  • it controls the visibility and it should not be stored in the local state, but instead a new property onOverlayVisibleChange (or other name) should be provided, so that the parent component can take the full responsibility of this state.

The current situation is halfway between the above and seems incorrect.

@melloware
Copy link
Member Author

@kalinkrustev the original use case was they wanted to have the panel open on render. so if overlayVisible={true} the overlay is displayed on render. However because of how the overlay positioning works the overlay is not in the right position and that is why the 100ms delay to allow the component to render and then position the overlay properly.

If you have a better solution a PR is welcome.

@kalinkrustev
Copy link
Contributor

So we aim for the initial visibility case? This should be reflected in the docs.

@melloware
Copy link
Member Author

Yes for initial visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Team Issue or pull request has been *opened* by a member of Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MultiSelect: Add "overlayVisible" property
2 participants