-
Notifications
You must be signed in to change notification settings - Fork 535
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
FilteredActionList inherits height and maxHeight from its parent #1500
Conversation
🦋 Changeset detectedLatest commit: fe18735 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 |
size-limit report 📦
|
@dusave was kind enough to verify that this fixes the bug in new projects |
After speaking with @vdepizzol, I wanted to offer a tweak on this: Remove the height/maxHeight styling from
edit: gonna do this |
<FilteredActionList | ||
filterValue={filterValue} | ||
onFilterChange={onFilterChange} | ||
{...listProps} |
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.
Since the FilteredActionListProps
interface now extends SxProp
, someone may pass a listProps
that includes sx
, right? If they do, line 166 below will overwrite the sx
value they pass. Would we want to merge (i.e. sx={{ ...listProps.sx, height: 'inherit', maxHeight: 'inherit' }}
)?
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 might have this wrong, but I think we're safe as is because the SelectPanelProps
type doesn't include an sx
prop, and listProps
is just a "rest" destructuring of the SelectPanel
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.
@jfuchs I might be missing something, but it seems that SelectPanelProps
does contain sx
, via Omit<FilteredActionListProps, 'selectionVariant'>
:
react/src/SelectPanel/SelectPanel.tsx
Lines 34 to 35 in c4926a2
export type SelectPanelProps = SelectPanelBaseProps & | |
Omit<FilteredActionListProps, 'selectionVariant'> & |
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.
Yep, sorry — you're right, it is in there. Updated!
This PR updates
FilteredActionList
so that its root HTML element is styled withheight: inherit; max-height: inherit
. This prevents a bug whereFilteredActionList
sizes itself larger than theOverlay
that contains it, preventing scrolling.Closes https://github.com/github/primer/issues/333
Screenshots
before:
CleanShot.2021-09-30.at.10.11.37.mp4
after:
CleanShot.2021-10-05.at.12.09.13.mp4
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.