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

Review "RestProps" spread method for arbitrary attributes #2703

Open
endigo9740 opened this issue Jun 3, 2024 · 3 comments
Open

Review "RestProps" spread method for arbitrary attributes #2703

endigo9740 opened this issue Jun 3, 2024 · 3 comments
Assignees
Labels
enhancement New feature or request feature request Request a feature or introduce and update to the project.
Milestone

Comments

@endigo9740
Copy link
Contributor

endigo9740 commented Jun 3, 2024

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.
@endigo9740 endigo9740 added the feature request Request a feature or introduce and update to the project. label Jun 3, 2024
@endigo9740 endigo9740 added this to the v3.0 (Next) milestone Jun 3, 2024
@endigo9740 endigo9740 self-assigned this Jun 3, 2024
@endigo9740 endigo9740 added the enhancement New feature or request label Jun 3, 2024
@ryceg
Copy link
Contributor

ryceg commented Jun 3, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

Hugos68 commented Jul 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request Request a feature or introduce and update to the project.
Projects
None yet
Development

No branches or pull requests

3 participants