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

[feature] Dynamic elements implementation <svelte:element> #6898

Merged
merged 73 commits into from
Apr 8, 2022
Merged

[feature] Dynamic elements implementation <svelte:element> #6898

merged 73 commits into from
Apr 8, 2022

Conversation

baseballyama
Copy link
Member

@baseballyama baseballyama commented Nov 3, 2021

Again I started to refactor the render_dom part. I think we should implement <svelte:element> as Element + dynamic tag completely for maintainability.
=> DONE.


This is based on #5481.
I cleaned up the code and few changes.

  • FIx few bugs.

    • Maintain attributes and events when changed <svelte:element>'s tag.
    • Fix rendering place when uses named slot.
  • Return compile error if svelte:element has animate because <svelte:element> doesn't support it.

  • Stop to use DynamicElement classes. Because...

    • I tried to create a base class but bundled javascript was broken due to circular dependency.
    • To use DynamicElement without extending makes much duplicate code, therefore I added a feature to Element which can have a tag dynamically.
  • Add more tests

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@baseballyama
Copy link
Member Author

The current implementation has one bug: The following code will not play the transition:
I spoke with the other maintainers and we agree that this is enough of an edge case that this limitation is okay for now.

I agree with you!

MEMO: complete code to reproduce this
<script>
  import {fade} from 'svelte/transition';
  let tag = "p";
  function click(newTag) {
    tag = newTag;
  }
</script>

tag: {tag}
<button on:click="{() => click('p')}">Click me! (-> p)</button>
<button on:click="{() => click('div')}">Click me! (-> div)</button>
<button on:click="{() => click(null)}">Click me! (-> null)</button>  

<svelte:element this={tag} transition:fade style="border: 1px solid #000">
  svelte:element tag
  {#if tag === 'p'}
    <p>foo</p>
  {/if}
</svelte:element>

{#if tag === "div"}
  <div transition:fade style="border: 1px solid #000">
    div tag
    {#if tag === 'p'}
      <p>foo</p>
    {/if}
  </div>
{/if}

{#if tag === "p"}
  <p transition:fade style="border: 1px solid #000">
    p tag
    {#if tag === 'p'}
      <p>foo</p>
    {/if}
  </p>
{/if}

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response! Your code snippet doesn't play the animation but it changes the tags and the order. To me this is just as much of and edge case as the transition bug, so I'm ok with that limitation for now and therefore think we should keep the animation support I added.
I'm therefore giving my approval on this PR. I'll try to get a second pair of eyes from another maintainer on this, soon.

@caesar-exploit
Copy link

I was just doing something similar, do you have an approximate date when this implementation will go into production?

@tanhauhau
Copy link
Member

Because I noticed that the animation does not work when changing a tag.

@baseballyama @dummdidumm ok i've made changes, this animates now.

@baseballyama
Copy link
Member Author

@tanhauhau

Thank you so much!

I think it may be necessary to change the structure of the runtime code.

I initially thought it would be better to leave it as a limitation rather than introduce a new method like restore_measurements in Block.ts, but I support the maintainer's decision.
(And this structural change was simpler than I thought👍 )
I learned from this code 💪 Thank you!

@dummdidumm dummdidumm merged commit e0d9325 into sveltejs:master Apr 8, 2022
@dummdidumm
Copy link
Member

Thank you @baseballyama, @alfredringstad, @tanhauhau and all the others for your invaluable help implementing this!

@baseballyama
Copy link
Member Author

@dummdidumm

Thank you so much for your big support too!! 💪💪

@lukaszpolowczyk
Copy link

Tutorial after clicking "Show me":
obraz

@0xedb
Copy link

0xedb commented Apr 8, 2022

Does Svelte have TypeScript type for the tags like React's React.ElementType and React.ComponentProps?

I want to achieve something akin to this with svelte:element: https://www.youtube.com/watch?v=uZ8GZm5KEXY

@antony
Copy link
Member

antony commented Apr 8, 2022 via email

@baseballyama
Copy link
Member Author

@lukaszpolowczyk

It works fine now!

@lukaszpolowczyk
Copy link

@baseballyama Thanks. :)

@oekazuma oekazuma mentioned this pull request Apr 9, 2022
4 tasks
@AlbertMarashi
Copy link

Woooh! Thank you everyone!

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.