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][Dialog] forceMount type for Overlay & Content #1060

Closed
wants to merge 3 commits into from
Closed

[Fix][Dialog] forceMount type for Overlay & Content #1060

wants to merge 3 commits into from

Conversation

joshuaellis
Copy link

Description

the forceMount prop on Dialog.Overlay and Dialog.Content both specify that the value should be boolean however the type signature has been declared as true therefore providing false creates a TS error.

Maybe the initial idea was that undefined could be used instead, but if you pair it up with onOpenChange from Dialog.Root you're only provided boolean values.

What

To solve this, I've replaced the true value with false.

@andy-hook
Copy link
Contributor

Hi @joshuaellis, forceMount is not intended to be dynamic and the typings here are to indicate that. Components should only be force mounted for their lifetime. The prop is intended primarily for usage with JS animation libs in order to handover mount handling.


Is there a reason you need to be pairing it with onOpenChange?

@joshuaellis
Copy link
Author

joshuaellis commented Jan 4, 2022

I suppose my use-case is rather niche and potentially goes against what the library should be used for:

I'm using Remix but stitches doesn't collect the styles correctly unless the component is initially mounted i.e. if you mount after the initial render it doesn't work (see stitchesjs/stitches#908). As a work around I can use useSpring to animate the component in and out and dynamically set the mounting of the component.

It'd be good to understand (if not for my own understanding) why forceMount is not intended to by dynamic, are there performance issues etc.?

You can see my "hacky" implementation here.

EDIT: I fixed my issue with stitches & remix. So, i'll just close this PR. But thank you for your time :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants