-
Notifications
You must be signed in to change notification settings - Fork 8
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
Slot "unwrap" strips tags unnecessarily (when not using shadow root) #12
Comments
🤦♂️ D'oh! I think I just figured it out. I think me writing this issue was like talking to a rubber duck.🦆 Turns out it's useful for default slots but not for the named slots. I'll go ahead and submit a PR for this in a bit. |
…ing shadow root. Only "unwrap" (i.e. from the host tag) when in the default slot. Including ignore of .idea/ files (JetBrains) and enforcing repo-specific whitespace rules (differs from my specific IDE settings)
…ing shadow root. (#13) * Fix for #12: Ensure original named slot tags are retained when not using shadow root. Only "unwrap" (i.e. from the host tag) when in the default slot. Including ignore of .idea/ files (JetBrains) and enforcing repo-specific whitespace rules (differs from my specific IDE settings) * Removing yarn.lock (added accidentally) * Fixing typo in .editorconfig
I'll do an npm release of this. Feel free to close when you're ready. |
…en not using shadow root. Take 2 required to account for new unit testing framework (Vitest) from PR #13.
…en not using shadow root. Take 2 required to account for new unit testing framework (Vitest) from PR #14.
Alright, just reopening for now since technically this issue is still present. Setup PR #15 to address this hopefully once and for all 😅 |
p.s. While I'm certain this is the correct thing to do moving forward, it might be worth discussing the potential that this could be a breaking change for some folks who may have somehow depended on this functionality, i.e.: where the named So, this could warrant a major version change. 🤔 What do you think? |
Fixed in forked package: Keeping issue + PR open since it's technically still an issue here (in case you still want to merge) 😊 |
I can see how styling could be effected by this. Will check with my own usage to see what happens. |
Hi @crisward. I'm investigating a solution to a bug with custom element handling in upcoming Svelte 4 sveltejs/svelte#8686 and I'm taking inspiration from
svelte-tag
which seems to address my issue (sans theunwrap
). I was wondering if you can recall the purpose of thisunwrap
function? 1e05665#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R86-R92Here's a REPL demonstrating how it's supposed to work (taken from docs): https://svelte.dev/repl/3e6e3ed3885b4efda808fcacd7263f0b?version=3.59.1 In this example the
<h1>
and<p>
tags remain. For reference, the generated HTML is:This functionality appears to be expected in
svelte-tag
based on the unit tests (e.g. this test). However, while the result should contain<div><div slot="inner">HERE</div></div>
), the unit test expects that interior slotted<div slot="inner">
to not exist. 🤔 REPL showing that here, too: https://svelte.dev/repl/a7414057d404440681ad17f31ba7e536?version=3.59.1The text was updated successfully, but these errors were encountered: