Skip to content

Update Button props to ensure they correctly type check - #2445

Merged
colebemis merged 8 commits into
mainfrom
button-type-check
Oct 25, 2022
Merged

Update Button props to ensure they correctly type check#2445
colebemis merged 8 commits into
mainfrom
button-type-check

Conversation

@mattcosta7

@mattcosta7mattcosta7 commented Oct 17, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Describe your changes here.

Discussed separately in slack

types for Button were being broadened signficantly because the generic interface exposed by the polymorphic component could not be adequately described by the base forwardRef implementation. Using the PolymorphicForwardRefComponent helper, allows the type fidelity to be maintained.

I've added a type test to validate a few simple cases, and updated places where the types were incorrect in the codebase (now easily noted as failures to build)

One thing to consider - this might be a breaking change for consumers, even though the api should have been this way already

fixes#1956

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • 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

changeset-botBot commented Oct 17, 2022•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: cc79c6c

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

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

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 temporarily deployed to github-pages October 17, 2022 21:45 Inactive
@github-actions

github-actionsBot commented Oct 17, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
dist/browser.esm.js78.12 KB (0%)
dist/browser.umd.js78.77 KB (0%)

@mattcosta7mattcosta7 changed the title button type checkUpdate Button props to ensure they correctly type checkOct 17, 2022
mattcosta7
mattcosta7 commented Oct 17, 2022
View reviewed changes
Comment threadsrc/ActionMenu.tsx
mattcosta7
mattcosta7 commented Oct 17, 2022
View reviewed changes
Comment threadsrc/Button/Button.tsx
mattcosta7
mattcosta7 commented Oct 17, 2022
View reviewed changes
Comment threadsrc/Button/types.ts
mattcosta7
mattcosta7 commented Oct 17, 2022
View reviewed changes
Comment threadsrc/drafts/MarkdownEditor/_ViewSwitch.tsx
mattcosta7
mattcosta7 commented Oct 17, 2022
View reviewed changes
Comment threadsrc/stories/ActionMenu/fixtures.stories.tsx
@mattcosta7mattcosta7 self-assigned this Oct 17, 2022
@mattcosta7
mattcosta7 marked this pull request as ready for review October 17, 2022 22:09
@mattcosta7
mattcosta7 requested review from a team and siddharthkpOctober 17, 2022 22:09
mattcosta7
mattcosta7 commented Oct 17, 2022
View reviewed changes
Comment threadsrc/_TextInputInnerAction.tsx
@mattcosta7
mattcosta7 temporarily deployed to github-pages October 17, 2022 22:17 Inactive
@mattcosta7

mattcosta7 commented Oct 17, 2022

Copy link
Copy Markdown
ContributorAuthor

Not sure why canary release seems to fail

@mattcosta7
mattcosta7 requested a review from jclemOctober 17, 2022 22:20
@siddharthkpsiddharthkp added react 💓collab a vibrant hub of collaboration labels Oct 18, 2022
@mattcosta7mattcosta7 mentioned this pull request Oct 18, 2022
Closed
@mattcosta7
mattcosta7 temporarily deployed to github-pages October 19, 2022 17:20 Inactive
@mattcosta7

mattcosta7 commented Oct 19, 2022

Copy link
Copy Markdown
ContributorAuthor

I tested the canary build on memex - https://github.com/github/memex/pull/12803

button now properly catches a bunch of type errors that were introduced in that codebase, without any false positives. Feeling pretty confident here

jclem
jclem reviewed Oct 20, 2022
View reviewed changes
Comment threadsrc/__tests__/Button.types.test.tsx
@mattcosta7
mattcosta7 temporarily deployed to github-pages October 20, 2022 14:02 Inactive
@mattcosta7mattcosta7 mentioned this pull request Oct 21, 2022
Closed
@mattcosta7
mattcosta7 temporarily deployed to github-pages October 25, 2022 12:52 Inactive
keithamus
keithamus approved these changes Oct 25, 2022
View reviewed changes

@keithamuskeithamus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

colebemis
colebemis approved these changes Oct 25, 2022
View reviewed changes

@colebemiscolebemis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@colebemis
colebemis merged commit 6a1a2bd into mainOct 25, 2022
@colebemis
colebemis deleted the button-type-check branch October 25, 2022 15:01
@primer-cssprimer-css mentioned this pull request Oct 25, 2022
Merged
@mattcosta7mattcosta7 mentioned this pull request Nov 28, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jfuchsjfuchsjfuchs left review comments
@joshblackjoshblackjoshblack approved these changes
@siddharthkpsiddharthkpAwaiting requested review from siddharthkp
+3 more reviewers
@keithamuskeithamuskeithamus approved these changes
@jclemjclemjclem approved these changes
@colebemiscolebemiscolebemis approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@mattcosta7mattcosta7

Labels

💓collaba vibrant hub of collaborationreact

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Inaccurate type definitions for Button variant prop

7 participants

@mattcosta7@jfuchs@keithamus@jclem@joshblack@colebemis@siddharthkp