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

Slot "unwrap" strips tags unnecessarily (when not using shadow root) #12

Open
patricknelson opened this issue Jun 2, 2023 · 6 comments

Comments

@patricknelson
Copy link
Contributor

patricknelson commented Jun 2, 2023

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 the unwrap). I was wondering if you can recall the purpose of this unwrap function? 1e05665#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R86-R92

Here'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:

<h1 slot="header">Hello</h1>
<p>Some content between header and footer</p>
<p slot="footer">Copyright (c) 2019 Svelte Industries</p>

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

@patricknelson
Copy link
Contributor Author

🤦‍♂️ 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.

patricknelson referenced this issue in patricknelson/svelte-retag Jun 2, 2023
…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)
crisward pushed a commit that referenced this issue Jun 3, 2023
…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
@crisward
Copy link
Owner

crisward commented Jun 3, 2023

I'll do an npm release of this. Feel free to close when you're ready.

patricknelson referenced this issue in patricknelson/svelte-retag Jun 5, 2023
…en not using shadow root. Take 2 required to account for new unit testing framework (Vitest) from PR #13.
@patricknelson patricknelson reopened this Jun 5, 2023
patricknelson referenced this issue in patricknelson/svelte-retag Jun 5, 2023
…en not using shadow root. Take 2 required to account for new unit testing framework (Vitest) from PR #14.
@patricknelson
Copy link
Contributor Author

Alright, just reopening for now since technically this issue is still present. Setup PR #15 to address this hopefully once and for all 😅

@patricknelson
Copy link
Contributor Author

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 <div slot=...> element wasn't introduced into the final result and instead only the contents were injected at the slot point.

So, this could warrant a major version change. 🤔 What do you think?

@patricknelson
Copy link
Contributor Author

Fixed in forked package: svelte-retag

Keeping issue + PR open since it's technically still an issue here (in case you still want to merge) 😊

@crisward
Copy link
Owner

I can see how styling could be effected by this. Will check with my own usage to see what happens.

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

No branches or pull requests

2 participants