- Notifications
You must be signed in to change notification settings - Fork1.1k
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.
| importMuiPopover,{ | ||
| typePopoverPropsasMuiPopoverProps, | ||
| // 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} | ||
| <Stackdirection="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
| 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.
| 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.
aslilac left a comment
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.
| importMuiPopover,{ | ||
| typePopoverPropsasMuiPopoverProps, | ||
| // 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.
| importtype{Branding}from"../branding"; | ||
| importcolorsfrom"../tailwindColors"; | ||
| exportdefault{ |
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} | ||
| > | ||
| <spanstyle={visuallyHidden}> (This is a</span> | ||
| {featureStageBadgeTypes[contentType]} | ||
| <spanstyle={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
| typeFeatureStageBadgeProps=Readonly< | ||
| Omit<HTMLAttributes<HTMLSpanElement>,"children">&{ | ||
| contentType:keyoftypeoffeatureStageBadgeTypes; | ||
| 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", | ||
| }asconstsatisfiesRecord<string,ReactNode>; | ||
| conststyles={ |
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