-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: Menu v3 #4782
feat: Menu v3 #4782
Conversation
🦋 Changeset detectedLatest commit: 8626b91 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 |
13ab254
to
659c72d
Compare
484d75f
to
d70601b
Compare
623a67a
to
5af99dd
Compare
packages/components/src/__actions__/Menu/v3/_docs/Menu.stories.tsx
Outdated
Show resolved
Hide resolved
f574e56
to
dee91f4
Compare
/** | ||
* A MenuTrigger adds open/close functionality when wrapping a Button and a Popover (with a Menu inside of the Popover) | ||
*/ | ||
export const MenuTrigger = (props: MenuTriggerProps): JSX.Element => ( |
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.
FYI the trigger components from RAC don't accept a ref
prop, so I haven't done a forwardRef for them
e60be6b
to
f320c4e
Compare
9c29bf9
to
33edde3
Compare
packages/components/src/__actions__/Menu/v3/MenuItem.module.scss
Outdated
Show resolved
Hide resolved
a1148a4
to
3b38a9d
Compare
@@ -4,7 +4,7 @@ import { | |||
exampleActionButtonPropsButton, | |||
exampleDropdownContentEnabled, | |||
exampleDropdownContentOneDisabled, | |||
} from "~components/Menu/_docs/examples" | |||
} from "~components/__actions__/Menu/v1/_docs/examples" |
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.
Oh this is hella confusing lol
@import "~@kaizen/design-tokens/sass/border"; | ||
@import "~@kaizen/design-tokens/sass/color"; | ||
@import "~@kaizen/design-tokens/sass/shadow"; | ||
@import "~@kaizen/design-tokens/sass/spacing"; | ||
@import "~@kaizen/design-tokens/sass/typography"; |
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.
Have you tried not using the SCSS syntax for this? I'm pretty sure it would work...
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.
Done
background-color: $color-white; | ||
color: $color-purple-800; | ||
width: 248px; | ||
max-height: 22rem; | ||
overflow: auto; | ||
padding-block: $spacing-6; | ||
outline: none; | ||
border-radius: $border-solid-border-radius; | ||
box-shadow: $shadow-large-box-shadow; |
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.
Also try convert these to CSS vars
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.
@mcwinter07 you might also want to try this, as a inch towards CSS modules
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.
Happy to give it a crack 💥 🔨
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.
Done
export * from "./Menu" | ||
export * from "./MenuItem" | ||
export * from "./MenuTrigger" | ||
// export * from "./SubmenuTrigger" |
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.
Given its not accessible I think we should just remove the SubmenuTrigger
and all related until we decide its a feature to have. I'm concerned we'll forget why this was commented out and let someone enable it by accident.
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.
Fair, I'll remove it all in a single commit once we're pretty settled on this PR going in, so that it's easy to see the code if we want to reference it later
c39ec2b
to
e17727c
Compare
c42484e
to
c1259c9
Compare
c1259c9
to
8626b91
Compare
Features pulled through:
Menu
component)Features blocked:
Not applicable / out of scope:
Notes: