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

Pass props through to ToggleSwitch wrapper #3520

Merged
merged 4 commits into from
Jul 14, 2023

Conversation

mattcosta7
Copy link
Collaborator

@mattcosta7 mattcosta7 commented Jul 14, 2023

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

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

@changeset-bot
Copy link

changeset-bot bot commented Jul 14, 2023

🦋 Changeset detected

Latest commit: c511800

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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

@mattcosta7 mattcosta7 changed the title Pass props through to toggle wrapper Pass props through to ToggleSwitch wrapper Jul 14, 2023
/** 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
Copy link
Collaborator Author

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

@mattcosta7 mattcosta7 marked this pull request as ready for review July 14, 2023 13:51
@mattcosta7 mattcosta7 requested review from a team and josepmartins July 14, 2023 13:51
@github-actions
Copy link
Contributor

github-actions bot commented Jul 14, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 101.94 KB (+0.03% 🔺)
dist/browser.umd.js 102.48 KB (+0.04% 🔺)

['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 {
Copy link
Collaborator Author

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

Copy link
Member

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 👀

Copy link
Collaborator Author

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

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 this pull request may close these issues.

2 participants