Uh oh!
There was an error while loading. Please reload this page.
feat(UnderlineNav): Support inset and flush variants - #5829
Conversation
🦋 Changeset detectedLatest commit: f93b66b 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 📦
|
Uh oh!
There was an error while loading. Please reload this page.
langermank
left a comment
There was a problem hiding this comment.
Heads up that we're about to remove the CSS Modules feature flag from UnderlinePanel early next week. I would hold off on this until that merges so that you don't need to extend this behavior to sx
Hey @maraisr, thanks for the PR! Is this ready for review once conflicts are resolved? |
maraisr
commented
Apr 28, 2025
Hi @lesliecdubs, I don't think its ready yet. Failing tests, conflicts etc. I have not had much time to get this PR up to shape, so if somebody on the team is looking to take this on, please do! 🚀 If not, I'll dedicate some time by the end of this week to wrap this up. 👍 I think its a great feature, and unlocks the removal of some |
| /* stylelint-disable-next-line primer/box-shadow */ | ||
| box-shadow: inset 0 -1px var(--borderColor-muted); | ||
| &:where([data-flush="false"], &[data-flush="false"] &) { |
There was a problem hiding this comment.
If you could add a small explanation of the selector that would be nice, not required
Uh oh!
There was an error while loading. Please reload this page.
hectahertz
commented
May 19, 2025
@maraisr if you have some time to make those changes I'd love to get this merged! |
inset and full variants
hectahertz
left a comment
There was a problem hiding this comment.
This looks great! Thanks for your contribution!! 🚀
I think flush would work better, as that's what we've used in other components. If you could please change that, then we can merge this :)
Approving in advance!
| ) | ||
| } | ||
| export const Full = () => { |
There was a problem hiding this comment.
| exportconstFull=()=>{ | |
| exportconstVariantFull=()=>{ |
| controls: { | ||
| expanded: true, | ||
| // variant and size are developed in the first design iteration but then they are abondened. | ||
| // size and others were developed in the first design iteration but then they are abandoned. |
inset and full variantsinset and flush variants👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/380428 |
🟢 golden-jobs completed with status |
Variant: flush
Variant: inset (default)
Changelog
New
Added
insetandfullas variant props toUnderlineNavto aid in removing horizontal spacing.Rollout strategy
Testing & Reviewing
Since the current behaviour is with inset spacing, the default for this new prop is also
inset. So fully backwards compatible, a testing strategy is that all existing vrt tests pass.Merge checklist