-
Notifications
You must be signed in to change notification settings - Fork 614
Add variant no-horizontal-padding to UnderlineNav #6086
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: c25ad3a 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! |
size-limit report 📦
|
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 a new variant
prop to the UnderlineNav
component, letting consumers disable its default horizontal padding.
- Introduces
variant
prop with'default'
and'no-horizontal-padding'
options - Applies conditional padding in CSS based on
data-variant
- Updates Storybook with controls and a dedicated “No Horizontal Padding” story
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/react/src/internal/components/UnderlineTabbedInterface.module.css | Removes default padding and adds conditional rules for the default variant |
packages/react/src/UnderlineNav/UnderlineNav.tsx | Defines variant prop, defaults it, and sets data-variant attribute |
packages/react/src/UnderlineNav/UnderlineNav.stories.tsx | Adds Storybook control for variant and default arg |
packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx | Adds a “No Horizontal Padding” feature story |
Comments suppressed due to low confidence (2)
packages/react/src/internal/components/UnderlineTabbedInterface.module.css:9
- Typo in comment:
accomodate
should be spelledaccommodate
.
/* using a box-shadow instead of a border to accomodate 'overflow-y: hidden' on UnderlinePanels */
packages/react/src/UnderlineNav/UnderlineNav.tsx:39
- New
variant
prop controls padding behavior but lacks corresponding unit or integration tests; consider adding tests to verify both variants (default
andno-horizontal-padding
) render correctly.
variant?: 'default' | 'no-horizontal-padding'
packages/react/src/internal/components/UnderlineTabbedInterface.module.css
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
One question I had was if we want the no padding case to be the default in the future or no 🤔 Put another way, is the default case that people should be removing this padding or not.
This came up when using components like Stack because it doesn't seem like most of the time you wouldn't want that padding in there. At a higher level, it's almost like we need a bleed concept aka https://github.com/github/primer/issues/786 for these types of moments
100%; I'm gonna change the variant 'default' value for now to indicate that it has padding, so we avoid future refactors no matter what we end up going for. If we end up going for a bleed component I'd be interested to debate the implementation, negative margins might make things messy. |
Closes https://github.com/github/primer/issues/4607
Continued from https://github.com/primer/react/pull/5829v
This pull request introduces a new
variant
prop to theUnderlineNav
component, allowing removal of the horizontal padding for tabs.Screen.Recording.2025-05-20.at.20.36.16.mov
Changelog
Changed
Component Enhancements:
variant
prop toUnderlineNav
with options'default'
and'no-horizontal-padding'
to control horizontal padding.Storybook Updates:
variant
prop to the Storybook controls forUnderlineNav
, allowing users to toggle between the two variants in the Storybook UI.Rollout strategy
Testing & Reviewing
Merge checklist