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

Upgrade svelte #90

Closed

Conversation

benjavicente
Copy link

This PR upgrades Svelte to use svelte:element in the Render component, removing the monstrosity that was needed to use the as={element} prop.

pd: thank you for the awesome port of the Headless UI 🚀

@vercel
Copy link

vercel bot commented Apr 24, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/rgossiaux/svelte-headlessui/6NkCMS8UYPUoJzvrHRJGfuyuwVem
✅ Preview: https://svelte-headlessui-git-fork-benjavicente-chore-dc24ec-rgossiaux.vercel.app

@benjavicente
Copy link
Author

benjavicente commented Apr 25, 2022

Some other notes:

  • isValidElement probably should be changed to something that validates if the as prop is a string or a component, like the official library.
  • as prop could have autocompletion with keyof HTMLElementTagNameMap instead of using a custom array.
  • There seems to be missing tests in of Render.svelte because svelte components are not tested as a as prop (all tests passed with svelte:component removed). In tests, toMatchInlineSnapshot is causing me some issues with indentation in the snapshots, so until that is fixed, this PR should not be merged.

Copy link
Owner

@rgossiaux rgossiaux left a comment

Choose a reason for hiding this comment

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

Thanks!

You need to update the peerDependencies to 3.47.0 as well, since users will need that for this to work. For that reason I haven't rushed to implementing this, so I'll probably merge your PR in a couple weeks (also gives us a little more time for bugs in svelte:element to be found).

@rgossiaux
Copy link
Owner

Oh, and yes, the TS types can be simplified now, as you mentioned--we should delete the hardcoded list of elements.

@rgossiaux
Copy link
Owner

Thanks for this PR @benjavicente ! I know I'm responding to it a little late :) I've switched over to svelte:element in the next branch (see #96) so I'm going to close this now. I like your changes to elements.ts but for some reason they make svelte-check take about 10x longer so I'm not incorporating them for now.

@rgossiaux rgossiaux closed this Jun 11, 2023
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.

2 participants