From 8efe767d00b608deb380458bf5c8c94b2d3f018d Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 8 Feb 2024 11:29:52 -0600 Subject: [PATCH 1/5] refactor(react): remove circular references --- packages/react/src/ActionList/Group.tsx | 2 +- packages/react/src/ActionList/Heading.tsx | 2 +- packages/react/src/ActionList/Item.tsx | 2 +- packages/react/src/ActionList/LinkItem.tsx | 2 +- packages/react/src/ActionList/List.tsx | 27 +------------------ packages/react/src/ActionList/Selection.tsx | 3 +-- packages/react/src/ActionList/index.ts | 2 +- packages/react/src/ActionList/shared.ts | 26 ++++++++++++++++++ packages/react/src/LabelGroup/LabelGroup.tsx | 5 +++- .../src/drafts/MarkdownEditor/Toolbar.tsx | 23 +++------------- .../drafts/MarkdownEditor/_SavedReplies.tsx | 2 +- .../drafts/MarkdownEditor/_ToolbarButton.tsx | 21 +++++++++++++++ 12 files changed, 62 insertions(+), 55 deletions(-) create mode 100644 packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx diff --git a/packages/react/src/ActionList/Group.tsx b/packages/react/src/ActionList/Group.tsx index 97a39107306..0eb9a9424e4 100644 --- a/packages/react/src/ActionList/Group.tsx +++ b/packages/react/src/ActionList/Group.tsx @@ -2,7 +2,7 @@ import React from 'react' import {useId} from '../hooks/useId' import Box from '../Box' import {SxProp, BetterSystemStyleObject, merge} from '../sx' -import {ListContext, ActionListProps} from './List' +import {ListContext, type ActionListProps} from './shared' import {AriaRole} from '../utils/types' import {default as Heading} from '../Heading' import type {ActionListHeadingProps} from './Heading' diff --git a/packages/react/src/ActionList/Heading.tsx b/packages/react/src/ActionList/Heading.tsx index 076d690df28..11f50436a51 100644 --- a/packages/react/src/ActionList/Heading.tsx +++ b/packages/react/src/ActionList/Heading.tsx @@ -4,7 +4,7 @@ import {defaultSxProp} from '../utils/defaultSxProp' import {useRefObjectAsForwardedRef} from '../hooks' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import {default as HeadingComponent} from '../Heading' -import {ListContext} from './List' +import {ListContext} from './shared' import VisuallyHidden from '../_VisuallyHidden' import {ActionListContainerContext} from './ActionListContainerContext' import {invariant} from '../utils/invariant' diff --git a/packages/react/src/ActionList/Item.tsx b/packages/react/src/ActionList/Item.tsx index a5736755e67..4b11f2695aa 100644 --- a/packages/react/src/ActionList/Item.tsx +++ b/packages/react/src/ActionList/Item.tsx @@ -12,7 +12,7 @@ import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/po import {ActionListContainerContext} from './ActionListContainerContext' import {Description} from './Description' import {GroupContext} from './Group' -import {ActionListProps, ListContext} from './List' +import {type ActionListProps, ListContext} from './shared' import {Selection} from './Selection' import {ActionListItemProps, getVariantStyles, ItemContext, TEXT_ROW_HEIGHT} from './shared' import {LeadingVisual, TrailingVisual, VisualProps} from './Visuals' diff --git a/packages/react/src/ActionList/LinkItem.tsx b/packages/react/src/ActionList/LinkItem.tsx index 978c040bf9f..91c38cfea63 100644 --- a/packages/react/src/ActionList/LinkItem.tsx +++ b/packages/react/src/ActionList/LinkItem.tsx @@ -4,7 +4,7 @@ import Link from '../Link' import {SxProp, merge} from '../sx' import {Item} from './Item' import {ActionListItemProps} from './shared' -import {Box} from '..' +import Box from '../Box' // adopted from React.AnchorHTMLAttributes type LinkProps = { diff --git a/packages/react/src/ActionList/List.tsx b/packages/react/src/ActionList/List.tsx index ba2178d3efa..809db5cbf77 100644 --- a/packages/react/src/ActionList/List.tsx +++ b/packages/react/src/ActionList/List.tsx @@ -8,32 +8,7 @@ import {defaultSxProp} from '../utils/defaultSxProp' import {useSlots} from '../hooks/useSlots' import {Heading} from './Heading' import {useId} from '../hooks/useId' - -export type ActionListProps = React.PropsWithChildren<{ - /** - * `inset` children are offset (vertically and horizontally) from `List`’s edges, `full` children are flush (vertically and horizontally) with `List` edges - */ - variant?: 'inset' | 'full' - /** - * Whether multiple Items or a single Item can be selected. - */ - selectionVariant?: 'single' | 'multiple' - /** - * Display a divider above each `Item` in this `List` when it does not follow a `Header` or `Divider`. - */ - showDividers?: boolean - /** - * The ARIA role describing the function of `List` component. `listbox` or `menu` are a common values. - */ - role?: AriaRole -}> & - SxProp - -type ContextProps = Pick & { - headingId?: string -} - -export const ListContext = React.createContext({}) +import {ListContext, type ActionListProps} from './shared' const ListBox = styled.ul(sx) diff --git a/packages/react/src/ActionList/Selection.tsx b/packages/react/src/ActionList/Selection.tsx index e47a5906b61..15c12070be6 100644 --- a/packages/react/src/ActionList/Selection.tsx +++ b/packages/react/src/ActionList/Selection.tsx @@ -1,8 +1,7 @@ import React from 'react' import {CheckIcon} from '@primer/octicons-react' -import {ListContext, ActionListProps} from './List' import {GroupContext, ActionListGroupProps} from './Group' -import {ActionListItemProps} from './shared' +import {type ActionListProps, type ActionListItemProps, ListContext} from './shared' import {LeadingVisualContainer} from './Visuals' import Box from '../Box' diff --git a/packages/react/src/ActionList/index.ts b/packages/react/src/ActionList/index.ts index 0bc89ae5986..a2638f7e1fc 100644 --- a/packages/react/src/ActionList/index.ts +++ b/packages/react/src/ActionList/index.ts @@ -7,7 +7,7 @@ import {Description} from './Description' import {LeadingVisual, TrailingVisual} from './Visuals' import {Heading} from './Heading' -export type {ActionListProps} from './List' +export type {ActionListProps} from './shared' export type {ActionListGroupProps} from './Group' export type {ActionListItemProps} from './shared' export type {ActionListLinkItemProps} from './LinkItem' diff --git a/packages/react/src/ActionList/shared.ts b/packages/react/src/ActionList/shared.ts index e69478eb17b..635eb08ed8b 100644 --- a/packages/react/src/ActionList/shared.ts +++ b/packages/react/src/ActionList/shared.ts @@ -108,3 +108,29 @@ export const getVariantStyles = ( } export const TEXT_ROW_HEIGHT = '20px' // custom value off the scale + +export type ActionListProps = React.PropsWithChildren<{ + /** + * `inset` children are offset (vertically and horizontally) from `List`’s edges, `full` children are flush (vertically and horizontally) with `List` edges + */ + variant?: 'inset' | 'full' + /** + * Whether multiple Items or a single Item can be selected. + */ + selectionVariant?: 'single' | 'multiple' + /** + * Display a divider above each `Item` in this `List` when it does not follow a `Header` or `Divider`. + */ + showDividers?: boolean + /** + * The ARIA role describing the function of `List` component. `listbox` or `menu` are a common values. + */ + role?: AriaRole +}> & + SxProp + +type ContextProps = Pick & { + headingId?: string +} + +export const ListContext = React.createContext({}) diff --git a/packages/react/src/LabelGroup/LabelGroup.tsx b/packages/react/src/LabelGroup/LabelGroup.tsx index c0f689978ec..4e2a0dd2642 100644 --- a/packages/react/src/LabelGroup/LabelGroup.tsx +++ b/packages/react/src/LabelGroup/LabelGroup.tsx @@ -4,7 +4,10 @@ import {XIcon} from '@primer/octicons-react' import {getFocusableChild} from '@primer/behaviors/utils' import {get} from '../constants' import VisuallyHidden from '../_VisuallyHidden' -import {AnchoredOverlay, Box, Button, IconButton, useTheme} from '..' +import {AnchoredOverlay} from '../AnchoredOverlay' +import Box from '../Box' +import {Button, IconButton} from '../Button' +import {useTheme} from '../ThemeProvider' import sx, {SxProp} from '../sx' export type LabelGroupProps = { diff --git a/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx b/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx index dccc1f35333..3a85a4d1974 100644 --- a/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx +++ b/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx @@ -12,33 +12,14 @@ import { QuoteIcon, TasklistIcon, } from '@primer/octicons-react' -import React, {forwardRef, memo, useContext, useRef} from 'react' +import React, {memo, useContext, useRef} from 'react' import {isMacOS} from '@primer/behaviors/utils' import Box from '../../Box' -import {IconButton, IconButtonProps} from '../../Button' import {useFocusZone} from '../../hooks/useFocusZone' import {MarkdownEditorContext} from './_MarkdownEditorContext' import {SavedRepliesButton} from './_SavedReplies' -export const ToolbarButton = forwardRef((props, ref) => { - const {disabled, condensed} = useContext(MarkdownEditorContext) - - return ( - e.preventDefault()} - {...props} - sx={{color: 'fg.muted', ...props.sx}} - /> - ) -}) -ToolbarButton.displayName = 'MarkdownEditor.ToolbarButton' - const Divider = () => { return ( { export const Toolbar = ({children}: {children?: React.ReactNode}) => {children} Toolbar.displayName = 'MarkdownEditor.Toolbar' + +export {ToolbarButton} from './_ToolbarButton' diff --git a/packages/react/src/drafts/MarkdownEditor/_SavedReplies.tsx b/packages/react/src/drafts/MarkdownEditor/_SavedReplies.tsx index d10b4b4ee5f..87ef8caa626 100644 --- a/packages/react/src/drafts/MarkdownEditor/_SavedReplies.tsx +++ b/packages/react/src/drafts/MarkdownEditor/_SavedReplies.tsx @@ -9,7 +9,7 @@ import React, { useState, } from 'react' import {SelectPanel, SelectPanelProps} from '../../SelectPanel' -import {ToolbarButton} from './Toolbar' +import {ToolbarButton} from './_ToolbarButton' export type SavedReply = { name: string diff --git a/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx b/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx new file mode 100644 index 00000000000..c36a353339b --- /dev/null +++ b/packages/react/src/drafts/MarkdownEditor/_ToolbarButton.tsx @@ -0,0 +1,21 @@ +import React, {forwardRef, useContext} from 'react' +import {IconButton, IconButtonProps} from '../../Button' +import {MarkdownEditorContext} from './_MarkdownEditorContext' + +export const ToolbarButton = forwardRef((props, ref) => { + const {disabled, condensed} = useContext(MarkdownEditorContext) + + return ( + e.preventDefault()} + {...props} + sx={{color: 'fg.muted', ...props.sx}} + /> + ) +}) +ToolbarButton.displayName = 'MarkdownEditor.ToolbarButton' From caa670e678b2095690fb8cf79731eb3137cf4208 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 8 Feb 2024 11:30:04 -0600 Subject: [PATCH 2/5] refactor(bundle): throw error when circular dependency is detected --- packages/react/rollup.config.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/react/rollup.config.js b/packages/react/rollup.config.js index 9e6bf971392..516a582ba8a 100644 --- a/packages/react/rollup.config.js +++ b/packages/react/rollup.config.js @@ -219,6 +219,10 @@ const baseConfig = { return } + if (warning.code === 'CIRCULAR_DEPENDENCY') { + throw warning + } + defaultHandler(warning) }, } From 01617e6ba9366d8dc2f23afd48b059b1b373b67e Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 8 Feb 2024 11:37:30 -0600 Subject: [PATCH 3/5] chore: add changeset --- .changeset/sharp-starfishes-battle.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/sharp-starfishes-battle.md diff --git a/.changeset/sharp-starfishes-battle.md b/.changeset/sharp-starfishes-battle.md new file mode 100644 index 00000000000..83b461fae00 --- /dev/null +++ b/.changeset/sharp-starfishes-battle.md @@ -0,0 +1,5 @@ +--- +'@primer/react': minor +--- + +Refactor project to avoid circular dependencies From abbae3b6291fb36af4b51e03cb2ec62c1ba98c72 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 8 Feb 2024 11:44:31 -0600 Subject: [PATCH 4/5] chore: fix toolbarbutton import --- packages/react/src/drafts/MarkdownEditor/Toolbar.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx b/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx index 3a85a4d1974..4fc6f8a7405 100644 --- a/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx +++ b/packages/react/src/drafts/MarkdownEditor/Toolbar.tsx @@ -19,6 +19,7 @@ import Box from '../../Box' import {useFocusZone} from '../../hooks/useFocusZone' import {MarkdownEditorContext} from './_MarkdownEditorContext' import {SavedRepliesButton} from './_SavedReplies' +import {ToolbarButton} from './_ToolbarButton' const Divider = () => { return ( @@ -151,4 +152,4 @@ export const CoreToolbar = ({children}: {children?: React.ReactNode}) => { export const Toolbar = ({children}: {children?: React.ReactNode}) => {children} Toolbar.displayName = 'MarkdownEditor.Toolbar' -export {ToolbarButton} from './_ToolbarButton' +export {ToolbarButton} From 3335d86d38ab0f672eba9c3109419a83571053be Mon Sep 17 00:00:00 2001 From: Josh Black Date: Thu, 8 Feb 2024 11:48:57 -0600 Subject: [PATCH 5/5] chore: fix eslint warnings --- packages/react/src/ActionList/List.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react/src/ActionList/List.tsx b/packages/react/src/ActionList/List.tsx index 809db5cbf77..578454192a4 100644 --- a/packages/react/src/ActionList/List.tsx +++ b/packages/react/src/ActionList/List.tsx @@ -2,7 +2,6 @@ import React from 'react' import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic' import styled from 'styled-components' import sx, {SxProp, merge} from '../sx' -import {AriaRole} from '../utils/types' import {ActionListContainerContext} from './ActionListContainerContext' import {defaultSxProp} from '../utils/defaultSxProp' import {useSlots} from '../hooks/useSlots'