-
-
Notifications
You must be signed in to change notification settings - Fork 206
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: data-state consistency and floating-ui styles #1042
fix: data-state consistency and floating-ui styles #1042
Conversation
🦋 Changeset detectedLatest commit: a33e894 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…lzak/melt-ui into fix/data-state-consistency
59ed884
to
aeaabec
Compare
…lzak/melt-ui into fix/data-state-consistency
In the link preview builder, we used to set the following properties on the content when I replaced Also, we always set effect([containSelection], () => {
...
contentElement.style.userSelect = 'text';
contentElement.style.webkitUserSelect = 'text';
}) The reason I removed all these styles and only kept Screen.Recording.2024-02-25.at.4.28.41.PM.mov |
Hey! Thanks again for all your work. if you could resolve conflicts, should be good to go! |
d9289bf
to
4b75856
Compare
4b75856
to
a33e894
Compare
Hey @TGlide! I resolved the conflicts. Thanks so much for your time! :) |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
closes #1041
There is currently an inconsistency in how we calculate the
data-state
attribute.With Dialog Content and Overlay, Tooltip Content:
With the rest of the components (Link Preview Content, Menu Content, Popover Content, etc.):
I believe that the
data-state
should be based on$open
, because if we useforceVisible: true
and always keep the content mounted,data-state
will always beopen
.The only place where this decision between
$open
and$isVisible
matters, is when the user usesforceVisible: true
, in which case if they unmount the component when$open
isfalse
and use a svelte transition,data-state
will actually stayopen
because of how svelte internally freezes the component to transition it out. But if the user always leaves it as mounted and just changes the content's visibility,data-state
should beclosed
, which is not currently the case with the codebase.Now, if we agree that we should change the
data-state
based on$open
, we need to make sure that we don't override styles set by floating-ui in certain components.The above code would "rerender" every time the
$open
state changes, and if the user never unmounts the content, the styles will get reset to an empty string, thereby removing the styles set by floating-ui, which causes the component to jump. This was already fixed in the case of the tooltip content here #1010The solution was to use the below code
... and to remove properties with undefined values returned from the tooltip content, because if we set style to undefined, svelte will also reset the styles set by floating-ui. So we needed to remove the style property altogether.
So since this will be a problem in all builders, we should remove properties with undefined values in the
makeElement
function.