-
Notifications
You must be signed in to change notification settings - Fork 535
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
Pass props through to ToggleSwitch wrapper #3520
Conversation
🦋 Changeset detectedLatest commit: c511800 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
/** The id of the DOM node that describes the switch */ | ||
['aria-describedby']?: string | ||
/** The id of the DOM node that labels the switch */ | ||
['aria-labelledby']: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are included in html attributes
size-limit report 📦
|
['aria-describedby']?: string | ||
/** The id of the DOM node that labels the switch */ | ||
['aria-labelledby']: string | ||
export interface ToggleSwitchProps extends Omit<React.HTMLAttributes<HTMLDivElement>, 'onChange'>, SxProp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be narrowed a bit more, but I don't think we need to do that (and i kind of wish we used the dom onchange event type too, but oh well).
We could also use BoxProps
here, but I don't think that gets us much since we're already explicitly passing through sx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a TS perspective, is there a convention we could use to decide when to use interface
with extends vs type
with intersection? Was curious what your read on this was and if the change here highlights when to best use one over the other 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any clear guidance, and there aren't many cases where it matters. I think it's a bit clearer to read as an interface extension here instead of x & { ..... }.& y
but entirely just a matter of perspective
Describe your changes here.
Pass dom props through to the toggle switch wrapper component, instead of swallowing them, and extend types to allow for this
Screenshots
Please provide before/after screenshots for any visual changes
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.