-
Notifications
You must be signed in to change notification settings - Fork 626
Add height, width and overflow to Popover component #6303
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
Conversation
🦋 Changeset detectedLatest commit: 6748644 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 adds height, width, and overflow properties to the Popover component to address consumer needs who were previously using AnchoredOverlay due to width restrictions. The changes provide more flexible layout options for popover content.
- Added width, height, and overflow props to PopoverContentProps with predefined size options
- Updated PopoverContent component to handle the new props via data attributes
- Added corresponding CSS rules for the new sizing and overflow behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
File | Description |
---|---|
packages/react/src/Popover/Popover.tsx | Added new props to PopoverContentProps type and updated PopoverContent component implementation |
packages/react/src/Popover/Popover.stories.tsx | Updated Storybook stories to demonstrate new props and added controls for testing |
packages/react/src/Popover/Popover.module.css | Added CSS rules for width, height, and overflow variants using data attributes |
packages/react/src/Popover/Popover.dev.stories.tsx | Updated dev story to demonstrate overflow behavior |
/* stylelint-disable-next-line primer/responsive-widths */ | ||
width: 640px; | ||
} | ||
&:where([data-width-xxlarge]) { |
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 CSS defines a xxlarge
width variant, but the TypeScript interface only includes xlarge
as the largest option. This creates an inconsistency where the CSS supports a size that cannot be accessed through the component API.
Copilot uses AI. Check for mistakes.
height: 256px; | ||
} | ||
&:where([data-height-medium]) { | ||
height: 320px; |
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.
There are trailing spaces after the semicolon on this line, which should be removed for consistency.
height: 320px; | |
height: 320px; |
Copilot uses AI. Check for mistakes.
@@ -33,6 +33,8 @@ Playground.args = { | |||
caret: 'top', | |||
open: true, | |||
relative: true, | |||
width: 'auto', |
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 default args set width: 'auto'
, but the component implementation sets width = 'small'
as the default. This inconsistency could confuse users about the actual default behavior.
width: 'auto', | |
width: 'small', |
Copilot uses AI. Check for mistakes.
@@ -33,6 +33,8 @@ Playground.args = { | |||
caret: 'top', | |||
open: true, | |||
relative: true, | |||
width: 'auto', | |||
height: 'auto', |
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 default args set height: 'auto'
, but the component implementation sets height = 'fit-content'
as the default. This inconsistency could confuse users about the actual default behavior.
height: 'auto', | |
height: 'fit-content', |
Copilot uses AI. Check for mistakes.
size-limit report 📦
|
c6834ab
to
f36ff28
Compare
7d10a6a
to
49ebc1e
Compare
8c9e1f0
to
ef8ec56
Compare
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.
LGTM, looks like the width changes on all our stories and is now slightly wider due to the 232 -> 256px change. I'd confirm this is ok with design before merging this since we'll experience similar changes in dotcom
.changeset/witty-paws-approve.md
Outdated
@@ -0,0 +1,5 @@ | |||
--- | |||
"@primer/react": patch |
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.
"@primer/react": patch | |
"@primer/react": minor |
just a suggestion 🤔
4459633
to
6b3daed
Compare
82df9b2
to
a5b1c3a
Compare
Closes https://github.com/github/primer/issues/4722
As described in the issue, we have consumers who are opting for
AnchoredOverlay
because ofwidth
restrictions in Popover.I added the width values as suggested. I have also added
height
andoverflow
properties as they are adjacent concerns.Rollout strategy
Testing & Reviewing
Merge checklist