Fix extra padding on SelectPanel with groups #6009
Conversation
This reverts commit b287a44.
🦋 Changeset detectedLatest commit: bc48c30 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! |
github-actions
bot
added
the
integration-tests: recommended
size-limit report 📦
|
…ng-selectpanel-groups
…ng-selectpanel-groups
francinelucca
added
the
update snapshots
…t-5869-hectahertz/fix-padding-selectpanel-groups
There was a problem hiding this comment.
Pull Request Overview
This PR reintroduces a previous patch to adjust list padding when groups are present by adding a new horizontal-inset variant and updating styles, docs, and stories accordingly.
- Introduce
horizontal-insetto thevariantunion in both deprecated and current ActionList props - Apply
horizontal-insetinSelectPanelwhen groups exist, and adjust padding/margin logic in ActionList components and CSS - Update stories, docs, and the changeset for the new variant
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| deprecated/ActionList/List.tsx | Extend variant type to include horizontal-inset |
| SelectPanel/SelectPanel.tsx | Pass horizontal-inset when groupMetadata exists |
| ActionList/shared.ts | Extend variant type to include horizontal-inset |
| ActionList/List.tsx | Split paddingY into top/bottom and handle new variant |
| ActionList/Item.tsx | Include horizontal-inset in marginX and width logic |
| ActionList/Heading.module.css | Add horizontal-inset to heading margin rules |
| ActionList/ActionList.stories.tsx | Add horizontal-inset to story controls |
| ActionList/ActionList.module.css | Adjust wrapper padding/margin for horizontal-inset |
| ActionList/ActionList.docs.json | Update docs to mention horizontal-inset and refine description |
| .changeset/fresh-lights-wink.md | Add changeset entry for the fix |
Comments suppressed due to low confidence (1)
packages/react/src/ActionList/shared.ts:123
- Add or update tests to cover the new
horizontal-insetvariant, verifying correct padding and margin behaviors in rendered output.
variant?: 'inset' | 'horizontal-inset' | 'full'
| paddingInlineStart: 0, // reset ul styles | ||
| paddingY: variant === 'inset' ? 2 : 0, | ||
| paddingTop: variant === 'inset' ? 2 : 0, | ||
| paddingBottom: variant === 'inset' || variant == 'horizontal-inset' ? 2 : 0, |
There was a problem hiding this comment.
Use strict equality (===) for the horizontal-inset variant check to match the existing pattern and avoid unintended coercion.
| paddingBottom: variant === 'inset' || variant == 'horizontal-inset' ? 2 : 0, | |
| paddingBottom: variant === 'inset' || variant === 'horizontal-inset' ? 2 : 0, |
| onInputRefChanged={onInputRefChanged} | ||
| placeholderText={placeholderText} | ||
| {...listProps} | ||
| variant={listProps.groupMetadata?.length ? 'horizontal-inset' : 'inset'} |
There was a problem hiding this comment.
Should we maybe just do this in list? and have the logic be internal?
There was a problem hiding this comment.
the list would have to override it's own variant prop and that feels a bit weird to me 🤔, there'd be no way for the consumer to override it
francinelucca
deleted the
revert-6008-revert-5869-hectahertz/fix-padding-selectpanel-groups
branch
Re-adds #5869 with some fixes
Closes https://github.com/github/primer/issues/4830
Changelog
Changed
Changes the
ActionListmode tofullwhen groups are present so we don't have that extra paddingRollout strategy
Testing & Reviewing
(with modern action list FF ONLY)
Before
After
Merge checklist