Review "RestProps" spread method for arbitrary attributes

[endigo9740]

Describe the feature in detail (code, mocks, or screenshots encouraged)

@ryceg has suggested we implement a spread prop that allows for overflow attributes to be appended to the root element in each component. Imagine something like this:

Svelte

let {
    foo = "bar",
    {...rest}
}: ExampleProps = $props();

<div ... {...rest}>...</div>

React

export const Switch: React.FC<SwitchProps> = ({
    foo = "bar",
    {...rest}
}) => { /* ... */ };

<div ... {...rest}>...</div>

Use Case

Appending ARIA attributes to the root element, for example:

<SomeComponent aria-lablelledby="foo" />

<div ... aria-lablelledby="foo">...</div>

What type of pull request would this be?

Enhancement

Provide relevant links or additional information.

We'll need to verify that this works as expected between both Svelte and React.

Potential Gotchas

• Duplicating standard props as attributes (ex: per above, foo is a prop, not an attribute)
• Folks using the standard class attribute over the base classes convention
• Folks thinking these attributes somehow extend into the children elements
• We should set contributor guidelines to prevent using rest props. These are for users ONLY.

[ryceg]

I don't think it's an unreasonable expectation that class be exposed and implemented on root elements. We're making building blocks, and keeping them to the regular HTML spec makes them predictable. We can always remove the class attribute via Omit<HTMLAttributes<HTMLDivElement>>, 'class'> to keep people using the classes convention if we decide to go all in on that, though.

Regarding duplicating the props, the last one wins- so just make sure that our spread operator is at the top of the element, and it'll be overridden properly.

[endigo9740]

Yeah we're pretty committed to the classes attribute because it allows for more than just the parent. Any children can follow the same pattern. So the mix of the standard class attribute + classes can and would cause some confusion I'm sure.

https://next.skeleton.dev/docs/resources/contribute/components#props

Though thinking it out, I don't think it would cause any real harm, but again, that's what I'll need to test to verify.

[Hugos68]

I agree with @ryceg. Using class instead of classes will probably be less confusing. I've already used the techniques decribe above for the Listbox PR: #2754 without realizing this issue was open. I did exactly what was described, spread the attributes and override the one's we use internally. Especially with the zag discussion coming up (#2777) it would be nice if we can resolve this before going all in into porting again.

[rChaoz]

I would like to bring some attention to this. Right now, I'm at an impasse, because I'm trying to disable the default <button> element introduced by the popover, and couldn't find a way. It would be great if, for components with multiple snippets, we could provide props as well, for example with triggerProps={{ disabled: true }}. Regular props can also be forwarded with baseProps instead of rest props, as rest props are used with Zag right now.

[endigo9740]

I'm going to go ahead and close this out, as we have a definitive answer to this in the upcoming Skeleton v4 release. I'd recommend checking out the updated Fundamentals docs and/or Contributor > Component docs for details when v4 launches for more information.