-
Notifications
You must be signed in to change notification settings - Fork 608
feat(UnderlineNav): Support inset
and flush
variants
#5829
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: eb68c08 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 📦
|
packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx
Outdated
Show resolved
Hide resolved
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.
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? |
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 |
@@ -11,6 +9,11 @@ | |||
/* using a box-shadow instead of a border to accomodate 'overflow-y: hidden' on UnderlinePanels */ | |||
/* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you could add a small explanation of the selector that would be nice, not required
@maraisr if you have some time to make those changes I'd love to get this merged! |
inset
and full
variants
Uh oh! @maraisr, at least one image you shared is missing helpful alt text. Check your pull request body to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/380428 |
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.
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!
@@ -145,3 +145,13 @@ export const CountersLoadingState = () => { | |||
</UnderlineNav> | |||
) | |||
} | |||
|
|||
export const Full = () => { |
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.
export const Full = () => { | |
export const VariantFull = () => { |
|
||
const meta: Meta<typeof UnderlineNav> = { | ||
title: 'Components/UnderlineNav', | ||
component: UnderlineNav, | ||
parameters: { | ||
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. |
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.
abondened
lol
inset
and full
variantsinset
and flush
variants
🔴 golden-jobs completed with status |
Variant: flush
Variant: inset (default)
Changelog
New
Added
inset
andfull
as variant props toUnderlineNav
to 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