Skip to content
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

Popover: Remove the caret #4978

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fuzzy-weeks-peel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': major
---

Popover: Remove `caret` from Popover
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 0 additions & 6 deletions packages/react/src/Popover/Popover.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,6 @@
"defaultValue": "div",
"description": "Sets the underlying HTML tag for the component"
},
{
"name": "caret",
"type": "| 'top' | 'bottom' | 'left' | 'right' | 'bottom-left' | 'bottom-right' | 'top-left' | 'top-right' | 'left-bottom' | 'left-top' | 'right-bottom' | 'right-top' ",
"defaultValue": "'top'",
"description": "Controls the position of the caret"
},
{
"name": "open",
"type": "boolean",
Expand Down
18 changes: 2 additions & 16 deletions packages/react/src/Popover/Popover.figma.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,9 @@ figma.connect(Popover, 'https://www.figma.com/design/GCvY3Qv8czRgZgvl1dG6lp/Prim
body: figma.string('body'),
heading: figma.string('heading'),
action: figma.children('Action'),
caret: figma.enum('caret', {
top: 'top',
'top-left': 'top-left',
'top-right': 'top-right',
bottom: 'bottom',
'bottom-left': 'bottom-left',
'bottom-right': 'bottom-right',
left: 'left',
'left-top': 'left-top',
'left-bottom': 'left-bottom',
right: 'right',
'right-top': 'right-top',
'right-bottom': 'right-bottom',
}),
},
example: ({caret, heading, body, action}) => (
<Popover caret={caret}>
example: ({heading, body, action}) => (
<Popover>
<Popover.Content>
<Heading
sx={{
Expand Down
22 changes: 1 addition & 21 deletions packages/react/src/Popover/Popover.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export default {
} as Meta<typeof Popover>

export const Default = () => (
<Popover relative open={true} caret="top">
<Popover relative open={true}>
<Popover.Content sx={{marginTop: 2}}>
<Heading sx={{fontSize: 2}}>Popover heading</Heading>
<Text as="p">Message about popovers</Text>
Expand All @@ -31,31 +31,11 @@ export const Playground: StoryFn<typeof Popover> = args => (
)

Playground.args = {
caret: 'top',
open: true,
relative: true,
}

Playground.argTypes = {
caret: {
control: {
type: 'radio',
},
options: [
'top',
'bottom',
'left',
'right',
'bottom-left',
'bottom-right',
'top-left',
'top-right',
'left-bottom',
'left-top',
'right-bottom',
'right-top',
],
},
open: {
control: {
type: 'boolean',
Expand Down
190 changes: 2 additions & 188 deletions packages/react/src/Popover/Popover.tsx
Original file line number Diff line number Diff line change
@@ -1,36 +1,17 @@
import {clsx} from 'clsx'
import styled from 'styled-components'
import {get} from '../constants'
import type {SxProp} from '../sx'
import sx from '../sx'
import type {ComponentProps} from '../utils/types'

type CaretPosition =
| 'top'
| 'bottom'
| 'left'
| 'right'
| 'bottom-left'
| 'bottom-right'
| 'top-left'
| 'top-right'
| 'left-bottom'
| 'left-top'
| 'right-bottom'
| 'right-top'

type StyledPopoverProps = {
/**
* @deprecated `caret` is deprecated and will be removed in v38.
*/
caret?: CaretPosition
relative?: boolean
open?: boolean
} & SxProp

const Popover = styled.div.attrs<StyledPopoverProps>(({className, caret = 'top'}) => {
const Popover = styled.div.attrs<StyledPopoverProps>(({className}) => {
return {
className: clsx(className, `caret-pos--${caret}`),
className,
}
})<StyledPopoverProps>`
position: ${props => (props.relative ? 'relative' : 'absolute')};
Expand All @@ -48,173 +29,6 @@ const PopoverContent = styled.div<SxProp>`
margin-left: auto;
padding: ${get('space.4')};
background-color: ${get('colors.canvas.overlay')};

// Carets
&::before,
&::after {
position: absolute;
left: 50%;
display: inline-block;
content: '';
}

&::before {
top: -${get('space.3')};
margin-left: -9px;
border: ${get('space.2')} solid transparent; // TODO: solid?
border-bottom-color: ${get('colors.border.default')};
}

&::after {
top: -14px;
margin-left: -${get('space.2')};
border: 7px solid transparent; // todo: solid
border-bottom-color: ${get('colors.canvas.overlay')};
}

// Bottom-oriented carets
${Popover}.caret-pos--bottom & ,
${Popover}.caret-pos--bottom-right & ,
${Popover}.caret-pos--bottom-left & {
&::before,
&::after {
top: auto;
border-bottom-color: transparent;
}

&::before {
bottom: -${get('space.3')};
border-top-color: ${get('colors.border.default')};
}

&::after {
bottom: -14px;
// stylelint-disable-next-line primer/borders
border-top-color: ${get('colors.canvas.overlay')};
}
}

// Top & Bottom: Right-oriented carets
${Popover}.caret-pos--top-right & ,
${Popover}.caret-pos--bottom-right & {
right: -9px;
margin-right: 0;

&::before,
&::after {
left: auto;
margin-left: 0;
}

&::before {
right: 20px;
}

&::after {
right: 21px;
}
}

// Top & Bottom: Left-oriented carets
${Popover}.caret-pos--top-left & ,
${Popover}.caret-pos--bottom-left & {
left: -9px;
margin-left: 0;

&::before,
&::after {
left: ${get('space.4')};
margin-left: 0;
}

&::after {
left: calc(${get('space.4')} + 1px);
}
}

// Right- & Left-oriented carets
${Popover}.caret-pos--right & ,
${Popover}.caret-pos--right-top & ,
${Popover}.caret-pos--right-bottom & ,
${Popover}.caret-pos--left & ,
${Popover}.caret-pos--left-top & ,
${Popover}.caret-pos--left-bottom & {
&::before,
&::after {
top: 50%;
left: auto;
margin-left: 0;
border-bottom-color: transparent;
}

&::before {
// stylelint-disable-next-line primer/spacing
margin-top: calc((${get('space.2')} + 1px) * -1);
}

&::after {
margin-top: -${get('space.2')};
}
}

// Right-oriented carets
${Popover}.caret-pos--right & ,
${Popover}.caret-pos--right-top & ,
${Popover}.caret-pos--right-bottom & {
&::before {
right: -${get('space.3')};
border-left-color: ${get('colors.border.default')};
}

&::after {
right: -14px;
// stylelint-disable-next-line primer/borders
border-left-color: ${get('colors.canvas.overlay')};
}
}

// Left-oriented carets
${Popover}.caret-pos--left & ,
${Popover}.caret-pos--left-top & ,
${Popover}.caret-pos--left-bottom & {
&::before {
left: -${get('space.3')};
border-right-color: ${get('colors.border.default')};
}

&::after {
left: -14px;
// stylelint-disable-next-line primer/borders
border-right-color: ${get('colors.canvas.overlay')};
}
}

// Right & Left: Top-oriented carets
${Popover}.caret-pos--right-top & ,
${Popover}.caret-pos--left-top & {
&::before,
&::after {
top: ${get('space.4')};
}
}

// Right & Left: Bottom-oriented carets
${Popover}.caret-pos--right-bottom & ,
${Popover}.caret-pos--left-bottom & {
&::before,
&::after {
top: auto;
}

&::before {
bottom: ${get('space.3')};
}

&::after {
bottom: calc(${get('space.3')} + 1px);
}
}

${sx};
`

Expand Down
32 changes: 2 additions & 30 deletions packages/react/src/__tests__/Popover.test.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import React from 'react'
import type {PopoverProps} from '../Popover'
import Popover from '../Popover'
import {render, behavesAsComponent, checkExports} from '../utils/testing'
import {render as HTMLRender} from '@testing-library/react'
import axe from 'axe-core'

const comp = (
<Popover caret="top" open>
<Popover open>
<Popover.Content>Hello!</Popover.Content>
</Popover>
)
Expand All @@ -24,41 +23,14 @@ describe('Popover', () => {

it('should have no axe violations', async () => {
const {container} = HTMLRender(
<Popover caret="top" open>
<Popover open>
<Popover.Content>Hello!</Popover.Content>
</Popover>,
)
const results = await axe.run(container)
expect(results).toHaveNoViolations()
})

const CARET_POSITIONS: PopoverProps['caret'][] = [
'top',
'bottom',
'left',
'right',
'bottom-left',
'bottom-right',
'top-left',
'top-right',
'left-bottom',
'left-top',
'right-bottom',
'right-top',
]

for (const pos of CARET_POSITIONS) {
it(`renders correctly for a caret position of ${pos}`, () => {
const element = (
<Popover caret={pos} open>
<Popover.Content>Hello!</Popover.Content>
</Popover>
)

expect(render(element)).toMatchSnapshot()
})
}

it('renders both elements as a <div>', () => {
expect(render(<Popover />).type).toEqual('div')
expect(render(<Popover.Content />).type).toEqual('div')
Expand Down
Loading
Loading