-
Notifications
You must be signed in to change notification settings - Fork 628
feat(Dialog): remove support for sx #6812
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: b3b1712 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
👋 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! |
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.
Pull Request Overview
This PR removes support for the sx
prop from the Dialog component and its sub-components as part of a major version release. The change aims to simplify the Dialog API by removing styled-system integration while maintaining backward compatibility through the @primer/styled-react
package.
- Removes
sx
prop support from Dialog, Dialog.Header, Dialog.Title, Dialog.Subtitle, Dialog.Body, and Dialog.Footer - Refactors internal usage of
Box
component to native HTML elements with CSS modules - Creates a styled-react wrapper to maintain
sx
support for consumers during migration
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
packages/styled-react/src/sx.ts | Adds utility function for styled-system CSS processing |
packages/styled-react/src/index.tsx | Updates exports to include Dialog from styled-react package |
packages/styled-react/src/experimental.tsx | Re-exports Dialog from local implementation instead of @primer/react |
packages/styled-react/src/components/Dialog.tsx | Implements Dialog wrapper with sx support using styled-components |
packages/styled-react/src/tests/primer-react.browser.test.tsx | Adds tests for Dialog sub-components with sx prop support |
packages/react/src/Dialog/Dialog.tsx | Removes sx prop and Box usage, replaces with native HTML elements |
packages/react/src/Dialog/Dialog.module.css | Adds CSS classes for header layout previously handled by Box |
.changeset/sweet-results-smell.md | Documents breaking change for major version release |
// @ts-expect-error - PrimerDialog.Header is not recognized as a valid component type | ||
return <Wrapper as={PrimerDialog.Header} ref={ref} {...props} /> |
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.
The TypeScript error suppression indicates a potential issue with the component typing. Consider properly typing the PrimerDialog sub-components or creating a more robust wrapper that doesn't require error suppression.
Copilot uses AI. Check for mistakes.
// @ts-expect-error - PrimerDialog.Body is not recognized as a valid component type | ||
return <Wrapper as={PrimerDialog.Body} ref={ref} {...props} /> |
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.
The TypeScript error suppression indicates a potential issue with the component typing. Consider properly typing the PrimerDialog sub-components or creating a more robust wrapper that doesn't require error suppression.
Copilot uses AI. Check for mistakes.
// @ts-expect-error - PrimerDialog.Footer is not recognized as a valid component type | ||
return <Wrapper as={PrimerDialog.Footer} ref={ref} {...props} /> |
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.
The TypeScript error suppression indicates a potential issue with the component typing. Consider properly typing the PrimerDialog sub-components or creating a more robust wrapper that doesn't require error suppression.
Copilot uses AI. Check for mistakes.
packages/react/src/Dialog/Dialog.tsx
Outdated
// TODO: get rid of typecasting `forwardRef` | ||
const Title = React.forwardRef<HTMLHeadingElement, StyledTitleProps>(function Title({className, ...rest}, forwardRef) { |
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.
The TODO comment suggests this is temporary code. Consider addressing the type casting issue or creating a tracking issue if this needs to be resolved in a future iteration.
// TODO: get rid of typecasting `forwardRef` | |
const Title = React.forwardRef<HTMLHeadingElement, StyledTitleProps>(function Title({className, ...rest}, forwardRef) { | |
const Title = React.forwardRef<HTMLHeadingElement, StyledTitleProps>(function Title( | |
{className, ...rest}, | |
forwardRef: React.ForwardedRef<HTMLHeadingElement>, | |
) { |
Copilot uses AI. Check for mistakes.
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 is just a comment I forgot to delete. Go ahead and remove it.
size-limit report 📦
|
packages/styled-react/src/index.tsx
Outdated
Dialog as PrimerDialog, | ||
type DialogProps as PrimerDialogProps, |
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'm confused by what's happening here too 😅
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 think the error is because we moved those imports to components/Dialog
and forgot to remove from here.
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.
Changes look good overall. Approved assuming you'll:
- fix the broken "default" story VRT that was broken by removing
Dialog
sx
props - remove the
Dialog
stories that use thesx
prop - fix the linting errors
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.
oops, forgot to select "Approve" radio
60acad7
to
19b0741
Compare
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/2669 |
🔴 ci completed with status |
Closes #6762
Changelog
New
Changed
Box
internally from @mperrotti's workRemoved
sx
from DialogRollout strategy
We will use the
@primer/styled-react
package to update usage downstream before rolling this change out