search

svelteuidev / svelteui

SvelteUI Monorepo
https://svelteui.dev
MIT License
1.28k stars 64 forks source link

Many components are missing catch-all property for `$$restProps` #350

Closed longnguyen2004 closed 1 year ago

longnguyen2004 commented 1 year ago

What package has an issue

@svelteuidev/core

A clear and concise description of what the bug is

I've noticed that many components are using $$restProps but doesn't have [x: string]: unknown in their prop type, which causes TypeScript to error out. I haven't checked all the components, but Menu.Item is one of them, and probably every component in this commit as well. I can make a PR that adds them in if you approve.

In which browser(s) did the problem occur?

No response

Steps To Reproduce

Use MenuItem, which is a Box under the hood, and pass in something like href

Do you know how to fix the issue

Yes

Are you willing to participate in fixing this issue and create a pull request with the fix

Yes

Relevant Assets

341102479_743692877431801_1853707743389398122_n

BeeMargarida commented 1 year ago

Those rest props should match the props that the base HTML element of the component can receive. In the case you mention, the type of Menu.Item is buggy, it should extend the HTMLButtonElement and not HTMLElement. Nice catch, this one definitely needs a fix.

https://github.com/svelteuidev/svelteui/blob/main/packages/svelteui-core/src/components/Menu/MenuItem/MenuItem.d.ts

Can you give examples of other components? It might be the same problem of using the wrong HTML element

longnguyen2004 commented 1 year ago

Maybe those other components that I've pointed out don't need extra props, but if I want Menu.Item to be an anchor, I can't put href on there because there's no corresponding property in the type. Changing it to inherit HTMLButtonElement doesn't work either, as it essentially forces Menu.Item to be a button, which is not what I want.

BeeMargarida commented 1 year ago

It's already a button by default (check prop root). I've made a commit with a fix for this particular problem. Do you know of other cases where this happens?

longnguyen2004 commented 1 year ago

Menu.Item is the first thing that comes up to me, the others are probably fine, since you're not expected to change the root of them anyway.

BeeMargarida commented 1 year ago

Should be fixed then 👍 I'll probably release a patch this week