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

Update types for IconButton, ActionMenu.Button, TabNav.Link and internal consumers #2677

Merged
merged 18 commits into from
Dec 27, 2022

Conversation

mattcosta7
Copy link
Collaborator

@mattcosta7 mattcosta7 commented Dec 10, 2022

Describe your changes here.

The types for these buttons have seemed incorrect for some time. This should correct these types and ensure they get proper autocompletion in IDEs. I'm not 100% sure if the polymorphism on AnchorMenu.Button is correct, but it's consistent with what the types purported to be previously as far as I can tell

fixes #2622
fixes #2519

Are there are, similar instances we should fixup while here?

I snuck in a small change more broadly, which replaces

const component = ({sx: sxProp = {}}) => {}
//with
const component = ({sx: sxProp = defaultSxProp}) => {}

This should avoid a new object allocation on every render of multiple components, and potentially memo/callback/effect re-evaluations related to them. I don't expect a significant perf improvement, but this should be a small one with no negative effects

These changes lead to many incompatible typings inside primer, which I've fixed up. I think there are likely many more typing inconsistencies in primer we should work to root out, but this should at least build as expected.

A consideration here is "Are these fixes broad enough to be considered breaking changes" as they'll likely require consumers to update code, similar to what we've had to do here

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.

@mattcosta7 mattcosta7 requested review from a team and joshblack December 10, 2022 13:37
@changeset-bot
Copy link

changeset-bot bot commented Dec 10, 2022

🦋 Changeset detected

Latest commit: 4761a20

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 Minor

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 update types for many buttony things update types for IconButton, ActionMenu.Button Dec 10, 2022
@mattcosta7 mattcosta7 changed the title update types for IconButton, ActionMenu.Button Update types for IconButton, ActionMenu.Button Dec 10, 2022
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 10, 2022 13:44 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 10, 2022 13:45 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 10, 2022 13:45 Inactive
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 10, 2022 13:52 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 10, 2022 13:52 Inactive
@mattcosta7 mattcosta7 marked this pull request as draft December 10, 2022 13:57
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 10, 2022 13:59 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 10, 2022 13:59 Inactive
@mattcosta7 mattcosta7 self-assigned this Dec 10, 2022
@mattcosta7 mattcosta7 marked this pull request as ready for review December 10, 2022 14:12
@mattcosta7 mattcosta7 changed the title Update types for IconButton, ActionMenu.Button Update types for IconButton, ActionMenu.Button, and internal consumers Dec 10, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 10, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 84.94 KB (+0.02% 🔺)
dist/browser.umd.js 85.55 KB (+0.02% 🔺)

src/Button/types.ts Outdated Show resolved Hide resolved
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 10, 2022 14:15 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 10, 2022 14:16 Inactive
children: React.ReactNode
} & ButtonBaseProps

export type IconButtonProps = ButtonA11yProps & {
icon: React.FunctionComponent<React.PropsWithChildren<IconProps>>
icon: React.ComponentType
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

broadens these to they work with arbitrary components, and not only primer octicons

@@ -105,9 +106,8 @@ const TabNavLink = styled.a.attrs<StyledTabNavLinkProps>(props => ({
}

${sx};
`
` as PolymorphicForwardRefComponent<'a', TabNavLinkProps>
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 is used polymorphically, but the styled-components as types conflict some with the polymorphic helper types - overriding the styled-components version gives better typing, and avoids those issues

}

export function shouldAcceptTabNavLinkprops() {
return <TabNav.Link to="to something" selected as={Button} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

validates that we can accept specific props, and as

@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 17, 2022 13:06 Inactive
@mattcosta7 mattcosta7 changed the title Update types for IconButton, ActionMenu.Button, and internal consumers Update types for IconButton, ActionMenu.Button, TabNav.Link and internal consumers Dec 17, 2022
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 17, 2022 13:12 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 17, 2022 13:12 Inactive
@mattcosta7
Copy link
Collaborator Author

@joshblack opened this one - https://github.com/github/github/pull/252052

Which fixes up the type errors in dotcom. We'd probably need to work with product teams to ensure the aria-labels added are good enough, but it does the heavy lifting.

This did uncover some additional fixes necessary to improve the as prop in TabNav.Link for a case used currently in dotcom

96a6224

and opens a new question related to a test.

This updates the types to support a test we have currently in this codebase. I think the test, incorrectly, accepts an href attribute for a div. The polymorphic as prop changes catch that. I've updated the types to support the test, but I think it might be more valid to update the test to avoid setting an href on a div.

bf1f55e

@mattcosta7 mattcosta7 temporarily deployed to github-pages December 19, 2022 20:41 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 19, 2022 20:42 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for taking this on @mattcosta7 and @joshblack! 💖

@mattcosta7 mattcosta7 temporarily deployed to github-pages December 27, 2022 18:20 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 27, 2022 18:21 Inactive
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 27, 2022 18:47 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 27, 2022 18:48 Inactive
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 27, 2022 19:05 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-2677 December 27, 2022 19:06 Inactive
@mattcosta7 mattcosta7 temporarily deployed to github-pages December 27, 2022 19:36 — with GitHub Actions Inactive
@mattcosta7 mattcosta7 merged commit d356be8 into main Dec 27, 2022
@mattcosta7 mattcosta7 deleted the fix-up-types-for-buttony-things branch December 27, 2022 20:22
@primer-css primer-css mentioned this pull request Dec 27, 2022
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.

IconButton and ActionMenu.Button types are incorrect Incorrect types for IconButton
4 participants