fixed popup not being able to find the correct target because client side rendering hasn't finished yet

[TheEisbaer]

Linked Issue

Closes #2465

Description

My attempt at implementing a mutation observer as mentioned in the issue

Changsets

Instructions: Changesets automate our changelog. If you modify files in /packages, run pnpm changeset in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should be minor while chores and bugfixes should be patch. Please prefix the changeset message with feat:, bugfix: or chore:.

Checklist

Please read and apply all contribution requirements.

• This PR targets the dev branch (NEVER master)
• Documentation reflects all relevant changes
• Branch is prefixed with: docs/, feat/, chore/, bugfix/
• Ensure Svelte and Typescript linting is current - run pnpm ci:check
• Ensure Prettier linting is current - run pnpm format
• All test cases are passing - run pnpm test
• Includes a changeset (if relevant; see above)

[changeset-bot]

🦋 Changeset detected

Latest commit: 756ed72

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

This PR includes changesets to release 1 package

Name	Type
@skeletonlabs/skeleton	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

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
skeleton-docs	✅ Ready (Inspect)	Visit Preview	Feb 28, 2024 6:58am

[endigo9740]

@TheEisbaer just FYI we've technically put popups into a feature freeze as we prepare for Skeleton v3. We plan to handle these very differently in the future. That said, I will make an exception since this looks to be a good fix. However, just note it may take a while for me to get a proper review in. Just wanted to give you a heads up.

[endigo9740]

Hey @TheEisbaer, unfortunately I am not able to accept your PR as I'm having trouble replicating the original issue as described.

In #2465, you mention that having a nested child layout and popup triggering within that, but it fails until you refresh the page. I can see this happening in your linked app on Stackblitz, however I cannot replicate it in a minimal reproduction (keyword: "minimal"). I'm sharing it here in case you wish to test it as well:

skeleton-popup-test.zip

To run it:

• Download and unzip the files
• Point your terminal at this project
• Run npm install (or similar)
• Run npm run dev to start a local server instance
• Note there's multiple popups and none replicate the issue as described

While I understand your app is not in a functional state, the issue with sharing your full application is it makes it much harder to isolate where things are going wrong. It could be some unforeseen combination. I'd encourage you to compare your code to mine and see if you can isolate the fault.

Until then I'm going to close this PR, but will gladly reopen or consider other PRs if you can help showcase where the issue occurs. Thanks.

[endigo9740]

@TheEisbaer my apologies for now getting back to this sooner. I've just been snowed in with other projects. While I think your change is good, there's a couple things I'm taking into consideration right now:

1. We're in the process of winding down on Skeleton v2 and Popup support in favor of Skeleton v3
2. We're planning to migrate everyone to Floating UI Svelte asap once Svelte 5 is available
3. I have some concerns about the mutation observer introducing potential side effects

Given all this, I think the best way forward is going to be to put this on ice for now. It's not that it's a bad or wrong change or implementation, mostly just bad timing to be honest. If you need this change, I suggest forking the feature and implementing locally, assuming you haven't already. That or try this work around.

FYI I'll be making a public announcement next week that I'm shifting the majority of effort from v2 -> v3, so this will likely make sense under that context. Either way, thanks for the contribution.