Fixing CSS in ButtonGroup for mixed types

.changeset/shy-squids-study.md

@@ -0,0 +1,5 @@
+---
+"@primer/react": patch
+---
+
+ButtonGroup CSS fixes for mixed types

e2e/components/ButtonGroup.test.ts

@@ -23,6 +23,10 @@ const stories = [
     title: 'SX Prop',
     id: 'components-buttongroup-dev--sx-prop',
   },
+  {
+    title: 'Overrides',
+    id: 'components-buttongroup-dev--link-button-with-icon-buttons',
+  },
 ] as const
 
 test.describe('ButtonGroup', () => {

packages/react/src/ButtonGroup/ButtonGroup.dev.stories.tsx

@@ -1,12 +1,68 @@
 import React from 'react'
 import type {Meta} from '@storybook/react'
 import ButtonGroup from './ButtonGroup'
-import {Button} from '../Button'
+import {Button, IconButton, LinkButton} from '../Button'
+import {CopilotIcon} from '@primer/octicons-react'
+import {Box, Tooltip, ThemeProvider, BaseStyles} from '..'
 
-export default {
+const meta: Meta<typeof ButtonGroup> = {
   title: 'Components/ButtonGroup/Dev',
   component: ButtonGroup,
-} as Meta<typeof ButtonGroup>
+  decorators: [
+    Story => {
+      // Add some padding to the wrapper box to make sure tooltip v1 is always in the viewport
+      return (
+        <ThemeProvider>
+          <BaseStyles>
+            <Box padding={5}>{Story()}</Box>
+          </BaseStyles>
+        </ThemeProvider>
+      )
+    },
+  ],
+}
+
+export default meta
+
+export const LinkAndButtonWithTooltip2 = () => (
+  <ButtonGroup sx={{pl: 2}}>
+    <Tooltip text="Additional info about the link">
+      <Button as="a" href="https://primer.style">
+        Link
+      </Button>
+    </Tooltip>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+  </ButtonGroup>
+)
+
+export const ButtonAndLinkWithTooltip2 = () => (
+  <ButtonGroup sx={{pl: 2}}>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+    <Tooltip text="Additional info about the link">
+      <Button as="a" href="https://primer.style">
+        Link
+      </Button>
+    </Tooltip>
+  </ButtonGroup>
+)
+
+export const ButtonGroupSingleButton = () => (
+  <ButtonGroup>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+    <div></div>
+  </ButtonGroup>
+)
+
+export const LinkButtonWithIconButtons = () => (
+  <ButtonGroup>
+    <LinkButton size="small" sx={{color: 'deeppink'}} href="https://primer.style">
+      Small link
+    </LinkButton>
+    <Button className="testCustomClassnameColor">Pink link</Button>
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+    <IconButton icon={CopilotIcon} aria-label="Open GitHub Copilot chat" />
+  </ButtonGroup>
+)
 
 export const SxProp = () => (
   <ButtonGroup sx={{border: '1px solid red'}}>

packages/react/src/ButtonGroup/ButtonGroup.features.stories.tsx

@@ -1,10 +1,11 @@
-import React, {useState} from 'react'
+import React from 'react'
 import type {Meta} from '@storybook/react'
 import ButtonGroup from './ButtonGroup'
 import {IconButton, Button} from '../Button'
 import {PlusIcon, DashIcon, TriangleDownIcon} from '@primer/octicons-react'
 import {ActionMenu} from '../ActionMenu'
 import {ActionList} from '../ActionList'
+import {Tooltip} from '../next'
 
 export default {
   title: 'Components/ButtonGroup/Features',
@@ -14,26 +15,84 @@ export default {
 export const IconButtons = () => (
   <ButtonGroup>
     {/* We can remove these unsafe props after we resolve https://github.com/primer/react/issues/4129 */}
-    {/* eslint-disable-next-line primer-react/a11y-remove-disable-tooltip */}
-    <IconButton unsafeDisableTooltip={true} icon={PlusIcon} aria-label="Add" />
-    {/* eslint-disable-next-line primer-react/a11y-remove-disable-tooltip */}
-    <IconButton unsafeDisableTooltip={true} icon={DashIcon} aria-label="Subtract" />
+    <IconButton icon={PlusIcon} aria-label="Add" />
+    <IconButton icon={DashIcon} aria-label="Subtract" />
   </ButtonGroup>
 )
 
 export const LoadingButtons = () => {
-  const [isLoading, setIsLoading] = useState(false)
-
-  const handleClick = () => {
-    setIsLoading(true)
-  }
+  const handleClick = () => {}
   return (
     <ButtonGroup>
-      <Button loading={isLoading} onClick={handleClick}>
+      <Button loading={true} onClick={handleClick}>
         Button 1
       </Button>
       <Button onClick={handleClick}>Button 2</Button>
-      <Button onClick={handleClick}>Button 3</Button>
+      <Tooltip text="Additional info about the button">
+        <Button onClick={handleClick}>Button 3</Button>
+      </Tooltip>
+    </ButtonGroup>
+  )
+}
+
+export const IconButtonsWithTooltip = () => (
+  <ButtonGroup>
+    <IconButton icon={PlusIcon} aria-label="Add" />
+    <IconButton icon={DashIcon} aria-label="Subtract" />
+  </ButtonGroup>
+)
+export const ButtonAndLink = () => (
+  <ButtonGroup>
+    <Button>Button</Button>
+    <Button as="a" href="https://primer.style">
+      Link
+    </Button>
+  </ButtonGroup>
+)
+
+export const LinksWithTooltip = () => (
+  <ButtonGroup>
+    <Tooltip text="Additonal text for link 1" direction="s">
+      <Button as="a" href="https://primer.style">
+        Sample Link 1
+      </Button>
+    </Tooltip>
+    <Tooltip text="Additonal text for link 2" direction="s">
+      <Button as="a" href="https://primer.style">
+        Sample Link 2
+      </Button>
+    </Tooltip>
+  </ButtonGroup>
+)
+
+export const InactiveButtonsGroup = () => {
+  const primaryButton = (
+    <Button inactive={true} aria-disabled={true} onClick={() => {}}>
+      Primary Button
+    </Button>
+  )
+
+  const secondaryButton = (
+    <IconButton aria-label="Secondary Button Aria Label" aria-disabled={true} inactive={true} icon={DashIcon} />
+  )
+
+  return (
+    <ButtonGroup>
+      <Tooltip text="This button is inactive" direction="ne">
+        {primaryButton}
+      </Tooltip>
+
+      <ActionMenu open={false} onOpenChange={() => {}}>
+        <ActionMenu.Anchor>
+          <Tooltip text="this button is inactive" direction="ne" type="label">
+            {secondaryButton}
+          </Tooltip>
+        </ActionMenu.Anchor>
+        <ActionMenu.Overlay align="end">
+          <ActionList.Item>Item 1</ActionList.Item>
+          <ActionList.Item>Item 2</ActionList.Item>
+        </ActionMenu.Overlay>
+      </ActionMenu>
     </ButtonGroup>
   )
 }

packages/react/src/ButtonGroup/ButtonGroup.module.css

@@ -3,31 +3,17 @@
   vertical-align: middle;
   isolation: isolate;
 
-  & > *:not([data-loading-wrapper]):is(button, a) {
+  & > *:not([data-loading-wrapper]) {
     /* stylelint-disable-next-line primer/spacing */
     margin-inline-end: -1px;
     position: relative;
-    border-radius: 0;
 
-    &:first-of-type {
-      border-top-left-radius: var(--borderRadius-medium);
-      border-bottom-left-radius: var(--borderRadius-medium);
+    /* reset border-radius */
+    button,
+    a {
+      border-radius: 0;
     }
 
-    &:last-of-type {
-      border-top-right-radius: var(--borderRadius-medium);
-      border-bottom-right-radius: var(--borderRadius-medium);
-    }
-
-    &:focus,
-    &:active,
-    &:hover {
-      z-index: 1;
-    }
-  }
-
-  /* if child is loading button */
-  [data-loading-wrapper] {
     &:first-child {
       button,
       a {
@@ -43,18 +29,54 @@
         border-bottom-right-radius: var(--borderRadius-medium);
       }
     }
+
+    &:focus,
+    &:active,
+    &:hover {
+      z-index: 1;
+    }
+  }
+
+  /* this is a workaround until portal based tooltips are fully removed from dotcom */
+  &:has(div:last-child:empty) {
+    button,
+    a {
+      border-radius: var(--borderRadius-medium);
+    }
   }
 
-  [data-loading-wrapper] > * {
+  /* if child is loading button */
+  & > *[data-loading-wrapper] {
     /* stylelint-disable-next-line primer/spacing */
     margin-inline-end: -1px;
     position: relative;
-    border-radius: 0;
+
+    /* reset border-radius */
+    button,
+    a {
+      border-radius: 0;
+    }
 
     &:focus,
     &:active,
     &:hover {
       z-index: 1;
     }
+
+    &:first-child {
+      button,
+      a {
+        border-top-left-radius: var(--borderRadius-medium);
+        border-bottom-left-radius: var(--borderRadius-medium);
+      }
+    }
+
+    &:last-child {
+      button,
+      a {
+        border-top-right-radius: var(--borderRadius-medium);
+        border-bottom-right-radius: var(--borderRadius-medium);
+      }
+    }
   }
 }

packages/react/src/ButtonGroup/ButtonGroup.tsx

@@ -1,4 +1,4 @@
-import React from 'react'
+import React, {type ComponentPropsWithRef} from 'react'
 import classes from './ButtonGroup.module.css'
 import {clsx} from 'clsx'
 import {FocusKeys, useFocusZone} from '../hooks/useFocusZone'
@@ -13,13 +13,15 @@ export type ButtonGroupProps = {
   role?: string
   /** className passed in for styling */
   className?: string
-} & SxProp
+} & SxProp &
+  ComponentPropsWithRef<'div'>
 
 const ButtonGroup = React.forwardRef<HTMLElement, ButtonGroupProps>(function ButtonGroup(
-  {className, role, sx, ...rest},
+  {className, role, sx, children, ...rest},
   forwardRef,
 ) {
   const buttonRef = useProvidedRefOrCreate(forwardRef as React.RefObject<HTMLDivElement>)
+  const buttons = React.Children.map(children, (child, index) => <div key={index}>{child}</div>)
 
   useFocusZone({
     containerRef: buttonRef,
@@ -30,11 +32,17 @@ const ButtonGroup = React.forwardRef<HTMLElement, ButtonGroupProps>(function But
 
   if (sx !== defaultSxProp) {
     return (
-      <Box as="div" className={clsx(className, classes.ButtonGroup)} role={role} {...rest} sx={sx} ref={buttonRef} />
+      <Box as="div" className={clsx(className, classes.ButtonGroup)} role={role} {...rest} sx={sx} ref={buttonRef}>
+        {buttons}
+      </Box>
     )
   }
 
-  return <div ref={buttonRef} className={clsx(className, classes.ButtonGroup)} role={role} {...rest} />
+  return (
+    <div ref={buttonRef} className={clsx(className, classes.ButtonGroup)} role={role} {...rest}>
+      {buttons}
+    </div>
+  )
 }) as PolymorphicForwardRefComponent<'div', ButtonGroupProps>
 
 ButtonGroup.displayName = 'ButtonGroup'