Skip to content

Prevent forced use of <fieldset> for individual fields. #108

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

Closed
willnationsdev opened this issue Nov 4, 2023 · 4 comments · Fixed by #109
Closed

Prevent forced use of <fieldset> for individual fields. #108

willnationsdev opened this issue Nov 4, 2023 · 4 comments · Fixed by #109

Comments

@willnationsdev
Copy link
Contributor

willnationsdev commented Nov 4, 2023

I noticed while using the package that TextField and SelectField both have a <fieldset> at the root while most other inputs have a <div>. This causes problems for me in a codebase I am attempting to use this package for since I need to be able to put multiple inputs/fields into one fieldset (and also include a <legend> as a header for that subset of fields).

Would it be possible to update those two (and any others I missed) to likewise swap <fieldset> for <div> so that users can handle that aspect themselves? Or if you don't want to break compatibility, perhaps use <svelte:element this={rootElement}> with an export let rootElement = "fieldset"?

I'd be happy to submit a PR myself if approved.

@willnationsdev willnationsdev changed the title Prevent forced used of <fieldset> for fields. Prevent forced use of <fieldset> for fields. Nov 4, 2023
@willnationsdev willnationsdev changed the title Prevent forced use of <fieldset> for fields. Prevent forced use of <fieldset> for individual fields. Nov 4, 2023
@techniq
Copy link
Owner

techniq commented Nov 4, 2023

@willnationsdev I don't have any strong reasons for keeping fieldset as the root instead of a standard div. Originally I thought it made more semantic sense, and was easier to target via CSS (and spot in dev tools), but I think it was an incorrect assumption. Since each component now has the component name as the class, most of those reasons are negated.

While a Field can have more than 1 control, in practice there isn't a guarantee. I wonder if it too should just become a div?

One of the main reason's TextField doesn't use Field internally is because of sveltejs/svelte#6059

So in short, I'm all for changing it to a div (and maybe instances) especially if it's causing you issues.

A PR would be awesome, and maybe someone else will chime in before we review/merge. Library is pre-1.0 for these types of revisions 😃

@techniq
Copy link
Owner

techniq commented Nov 4, 2023

Heads up, Field also exposes a fieldset slot, which is used by MenuField. Would need to come up with a better name for this :)

@willnationsdev
Copy link
Contributor Author

Heads up, Field also exposes a fieldset slot, which is used by MenuField. Would need to come up with a better name for this :)

Hmmm. Perhaps "root", "container", or "shell"?

@techniq
Copy link
Owner

techniq commented Nov 4, 2023

I like root, which aligns with classes.root and is used through Svelte UX to style the top-most element (and classes.container can be used within a component like Field does :)). Thanks

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 a pull request may close this issue.

2 participants