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: data-state consistency and floating-ui styles #1042

Merged
merged 12 commits into from
Mar 27, 2024

Conversation

anatolzak
Copy link
Contributor

@anatolzak anatolzak commented Mar 3, 2024

closes #1041

There is currently an inconsistency in how we calculate the data-state attribute.

With Dialog Content and Overlay, Tooltip Content:

'data-state': $open ? 'open' : 'closed'

With the rest of the components (Link Preview Content, Menu Content, Popover Content, etc.):

'data-state': $isVisible ? 'open' : 'closed'

I believe that the data-state should be based on $open, because if we use forceVisible: true and always keep the content mounted, data-state will always be open.

The only place where this decision between $open and $isVisible matters, is when the user uses forceVisible: true, in which case if they unmount the component when $open is false and use a svelte transition, data-state will actually stay open 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 be closed, 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.

'data-state': $open ? 'open' : 'closed',
style: styleToString({
    display: $isVisible ? undefined : 'none',
}),

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 #1010

The solution was to use the below code

'data-state': $open ? 'open' : 'closed',
style: $isVisible ? undefined : styleToString({ display: 'none' }),

... 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.

Copy link

changeset-bot bot commented Mar 3, 2024

🦋 Changeset detected

Latest commit: a33e894

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@melt-ui/svelte Patch

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

@anatolzak
Copy link
Contributor Author

anatolzak commented Mar 5, 2024

In the link preview builder, we used to set the following properties on the content when $isVisible is false.
hidden: true, style: { opacity: 0, pointer-events: none }

I replaced opacity: 0, pointer-events: none with display: none because when we set hidden: true, the element is already removed from document flow, so there is no reason to set the opacity and pointer events, and can instead just set display: none.

Also, we always set userSelect: none on the link preview content. It seems like it's unnecessary since we only need to worry about that during user selection which is already handled by

effect([containSelection], () => {
    ...
    contentElement.style.userSelect = 'text';
    contentElement.style.webkitUserSelect = 'text';
})

The reason I removed all these styles and only kept style: $isVisible ? undefined : styleToString({ display: 'none' }) is because if we set any styles when $isVisible is true and when forceVisible is false, we will rerender the styles every time $open changes and thus we reset the styles set by floating-ui which was causing the following issue

Screen.Recording.2024-02-25.at.4.28.41.PM.mov

@TGlide
Copy link
Member

TGlide commented Mar 26, 2024

Hey! Thanks again for all your work. if you could resolve conflicts, should be good to go!

@anatolzak
Copy link
Contributor Author

Hey @TGlide! I resolved the conflicts. Thanks so much for your time! :)

Copy link
Contributor

github-actions bot commented Mar 27, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
melt-ui ✅ Ready (View Log) Visit Preview a33e894

@huntabyte huntabyte changed the title fix data-state consistency and overriding floating-ui styles #1041 fix: data-state consistency and floating-ui styles Mar 27, 2024
@huntabyte huntabyte merged commit 2ed3d77 into melt-ui:develop Mar 27, 2024
6 checks passed
@github-actions github-actions bot mentioned this pull request Mar 27, 2024
@anatolzak anatolzak deleted the fix/data-state-consistency branch March 27, 2024 14:35
lolcabanon pushed a commit to lolcabanon/melt-ui that referenced this pull request Apr 20, 2024
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.

Data State Inconsistency
3 participants