Skip to content

fix(Banner): rewrite PrimaryAction & SecondaryAction types - #5055

Merged
francinelucca merged 3 commits into
mainfrom
francinelucca/3967-bug-bannerprimaryaction-and-bannersecondaryaction-button-to-support-href
Oct 8, 2024
Merged

fix(Banner): rewrite PrimaryAction & SecondaryAction types#5055
francinelucca merged 3 commits into
mainfrom
francinelucca/3967-bug-bannerprimaryaction-and-bannersecondaryaction-button-to-support-href

Conversation

@francinelucca

@francineluccafrancinelucca commented Oct 1, 2024

Copy link
Copy Markdown
Member

Closeshttps://github.com/github/primer/issues/3967

Modifies the Banner.PrimaryAction and Banner.SecondaryAction component types to solve for current type error when attempting to use as prop.

Changelog

Changed

Updates Banner.PrimaryAction & Banner.SecondaryAction to export as a PolymorphicForwardRefComponent to allow for better type inference and usage of the as prop.

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

If desired, you may render a Banner in your local environment to test with:

 primaryAction={<Banner.PrimaryAction as={Link} href="" />}
secondaryAction={
<Banner.SecondaryAction as={Link} href="">
Button
</Banner.SecondaryAction>
}

Merge checklist

@changeset-bot

changeset-botBot commented Oct 1, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ced3929

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

@github-actions

github-actionsBot commented Oct 1, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js96.78 KB (0%)
packages/react/dist/browser.umd.js97.08 KB (0%)

</Button>
)
}
}) as PolymorphicForwardRefComponent<'button', BannerPrimaryActionProps>

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don't think we necessarily care about forwarding a ref here but it's the only way I found to get the type-checking to pass (mimicked Button). Open to suggestions

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It makes sense to me, I'd do the same but yeah open to be convinced otherwise 😅

@primer-integration

Copy link
Copy Markdown

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/344960

@github-actions
github-actionsBottemporarily deployed to storybook-preview-5055 October 1, 2024 17:05 Inactive
@francinelucca
francinelucca marked this pull request as ready for review October 4, 2024 15:10
@francinelucca
francinelucca requested a review from a team as a code ownerOctober 4, 2024 15:10

@broccolinisoupbroccolinisoup left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥

</Button>
)
}
}) as PolymorphicForwardRefComponent<'button', BannerPrimaryActionProps>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It makes sense to me, I'd do the same but yeah open to be convinced otherwise 😅

…nd-bannersecondaryaction-button-to-support-href
@github-actions

Copy link
Copy Markdown
Contributor

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Oct 8, 2024
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5055 October 8, 2024 16:35 Inactive
@francineluccafrancinelucca added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Oct 8, 2024
@francinelucca
francinelucca added this pull request to the merge queueOct 8, 2024
Merged via the queue into main with commit 73135c1Oct 8, 2024
@francinelucca
francinelucca deleted the francinelucca/3967-bug-bannerprimaryaction-and-bannersecondaryaction-button-to-support-href branch October 8, 2024 18:09
@primerprimerBot mentioned this pull request Oct 8, 2024
@primerprimerBot mentioned this pull request Oct 18, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: Bannerintegration-tests: passingChanges in this PR do NOT cause breaking changes in gh/ghstaffAuthor is a staff memberstatus: review needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@francinelucca@broccolinisoup