- Notifications
You must be signed in to change notification settings - Fork923
feat: create UI badges for labeling beta features#14661
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -1,10 +1,12 @@ | |||
| import MuiPopover, { | |||
| type PopoverProps as MuiPopoverProps, | |||
| // biome-ignore lint/nursery/noRestrictedImports:Used asbase component | |||
| // biome-ignore lint/nursery/noRestrictedImports:This is thebase component that our custom popover is based on | |||
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.
I might have Biome configured wrong because my Biome plugin is flagging all of these comments as not doing anything
Accidentally removed this at first, but that's definitely a me problem, right?
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.
I think it doesn't like that our biome.json config is in the site/ directory 🙄 I hate that extensions never bother to handle subdirectories well.
| {title} | ||
| </h1> | ||
| {tooltip} | ||
| <Stack direction="row" spacing={2} alignItems="center"> |
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.
Only change is that the previous content is in a new stack so that the badges can be placed inside them
| @@ -149,7 +149,7 @@ const styles = { | |||
| menuItem: (theme) => css` | |||
| text-decoration: none; | |||
| color: inherit; | |||
| gap:20px; | |||
| gap:8px; | |||
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 was a change from when the feature badges were still part of the dropdown. Even with that removed, though, the gap seemed way too big for any feature elements we add
Uh oh!
There was an error while loading.Please reload this page.
| @@ -396,7 +412,7 @@ const classNames = { | |||
| `, | |||
| subLink: (css, theme) => css` | |||
| color:inherit; | |||
| color:${theme.palette.text.secondary}; | |||
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.
De-emphasized the colors of the inactive sub links because in dark mode, there wasn't enough contrast to make it obvious which subheader was active
Uh oh!
There was an error while loading.Please reload this page.
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.
I'm not sure I'm sold on whypreview needs to become anInteractiveRole, that's a lot of extra colors to manage for a badge that never uses thedisabled variants (and I don't get why it needshover either. we don't modify the colors of any other UI elements just because they have a tooltip.)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -1,10 +1,12 @@ | |||
| import MuiPopover, { | |||
| type PopoverProps as MuiPopoverProps, | |||
| // biome-ignore lint/nursery/noRestrictedImports:Used asbase component | |||
| // biome-ignore lint/nursery/noRestrictedImports:This is thebase component that our custom popover is based on | |||
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.
I think it doesn't like that our biome.json config is in the site/ directory 🙄 I hate that extensions never bother to handle subdirectories well.
| @@ -1,7 +1,7 @@ | |||
| import type { Branding } from "../branding"; | |||
| import colors from "../tailwindColors"; | |||
| export default { | |||
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.
Forgot to leave a comment on these earlier, but the reason why I switched the syntax fromsatisfies to type annotations is because when I was adding additional properties to the type definitions,satisfies wasn't always immediately kicking up an error when something was only partially updated, and it made way more red squiggles than it needed to
| {({ isOpen }) => ( | ||
| <span | ||
| css={[ | ||
| styles.badge, | ||
| size === "sm" && styles.badgeSmallText, | ||
| size === "lg" && styles.badgeLargeText, | ||
| isOpen && styles.badgeHover, | ||
| ]} | ||
| {...delegatedProps} | ||
| > | ||
| <span style={visuallyHidden}> (This is a</span> | ||
| {featureStageBadgeTypes[contentType]} | ||
| <span style={visuallyHidden}> feature)</span> | ||
| </span> | ||
| )} |
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.
Ended up using render props to cut down the repetitive state
| type FeatureStageBadgeProps = Readonly< | ||
| Omit<HTMLAttributes<HTMLSpanElement>, "children"> & { | ||
| contentType: keyof typeof featureStageBadgeTypes; | ||
| size?: "sm" | "md" | "lg"; | ||
| } | ||
| >; |
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.
Ended up applying YAGNI to the props and removed pretty much everything that isn't part of the current requirements. We have no need for a static version of the component right now
| experimental: "experimental", | ||
| } as const satisfies Record<string, ReactNode>; | ||
| const styles = { |
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.
I think we generally definestyles below the component
661d226 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#14499
Changes made
FeatureStageBadgecomponent for labeling components that are beta/experimentalPopovercomponent to support sending events outside the componentBonus
Notes